Handle key errors gracefully in a static queue

[zarifmahfuz]

The static queue executes tests during bisect and it leads to an incorrect outcome plus unexpected errors when we specify a non-existing test ID in the test order provided to the bisect command. We want to start bisecting test orders in main to reduce false positive outcomes. So several test IDs in a given flaky test order that was captured in an older commit could now be missing due to change in test file path / renamed test / deleted test. If we don't handle missing keys gracefully, bisect incorrect thinks that the static queue came across a test failure, which leads to incorrect outcomes.

[casperisfine]

when we specify a non-existing test ID in the test order provided to the bisect command

But why do we do that?

Also if there is a legitimate reason to provide non-existing IDs, then it's not the queue responsability to ignore these, but the runner.

[ChrisBr]

But why do we do that?

Also if there is a legitimate reason to provide non-existing IDs, then it's not the queue responsability to ignore these, but the runner.

I agree with Jean, I don't think we can or should just ignore key errors.

[ChrisBr]

IMO if a test id doesn't exist the bisect is invalid and can't be trusted.

[zarifmahfuz]

But why do we do that?

@casperisfine So there is usually some delay between when we consume a failing test and when we trigger a leakbot bisect on the test order that failed the test. This delay is between 1-3 days because we have a lot of failing test orders to analyze and we cap to 50 leakbot bisects per hour. In those 1-3 days of delay, somes test ids could have changed due to change in test file path / renamed test / deleted test.

[zarifmahfuz]

IMO if a test id doesn't exist the bisect is invalid and can't be trusted.

Why would it be invalid? Bisect could still isolate a leak if the non-existing test id does not raise a missing test id error.

[zarifmahfuz]

@casperisfine to further advocate, we've been seeing a lot of false positives of bisects lately and large portion of them are due to the fact that that we run the bisects on a branch that is older than a commit where a fix is made. Always running bisects on main would get rid of a large portion of the false positives that we've been experiencing.

We could add some alternate logic on the client side that compares git commits between a bisect was caught vs when a potential fix could've been made to main but that easily gets complicated and hard to get right.

[ChrisBr]

We could add some alternate logic on the client side that compares git commits between a bisect was caught vs when a potential fix could've been made to main but that easily gets complicated and hard to get right.

I don't think it needs to be that complicated, you can just alter the test order file.

# load_all_tests

if ENV["REMOVE_NOT_EXISTING_TESTS"]
  loaded_tests = Minitest::Test.runnables.flat_map do |runnable|
    runnable.runnable_methods.map do |method_name|
      "#{runnable}##{method_name}"
    end
  end

  tests_to_bisect = File.read("test_order.log").lines.map(&:squish)

  test_order = tests_to_bisect & loaded_tests

  puts "Removing #{tests_to_bisect.size - test_order.size} tests that no longer exist"

  File.write("test_artifacts/test_order.log", test_order)
end

[casperisfine]

In those 1-3 days of delay, somes test ids could have changed due to change in test file path / renamed test / deleted test.

It sounds to me that the solution should be for the leakbot build to use the same app revision than the failure report.

[ChrisBr]

It sounds to me that the solution should be for the leakbot build to use the same app revision than the failure report.

Yeah we do that for most part but sometimes want to check on main if the same issue still exists.