Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully handle non-existing failing test given to bisect #278

Merged
merged 1 commit into from
May 27, 2024

Conversation

zarifmahfuz
Copy link
Contributor

@zarifmahfuz zarifmahfuz commented May 10, 2024

Since we started periodically reproducing state leaks, I saw some cases where the failing test does not exist anymore. In such cases, the bisect command thinks that the failing test independently fails because the run_tests_in_fork(queue.failing_test) returns false -

--- Testing the failing test in isolation
/tmp/bundle/ruby/3.4.0+0/gems/ci-queue-0.52.0/lib/ci/queue/static.rb:91:in 'Hash#fetch': key not found: "Sales::ApiOrderToOrderServiceTest#test_an_order_... (KeyError)
--
  | Did you mean?  "Sales::OrderEditing::LineItemBuilderTest#test_some_other_test"
  | from /tmp/bundle/ruby/3.4.0+0/gems/ci-queue-0.52.0/lib/ci/queue/static.rb:91:in 'CI::Queue::Static#poll'
  | from /tmp/bundle/ruby/3.4.0+0/gems/ci-queue-0.52.0/lib/minitest/queue.rb:229:in 'Minitest::Queue#run_from_queue'
  | from /tmp/bundle/ruby/3.4.0+0/gems/ci-queue-0.52.0/lib/minitest/queue.rb:214:in 'Minitest::Queue#__run

...
The test fail when ran alone, no need to bisect.

This PR gracefully handles the case when the failing test does not exist.

@zarifmahfuz zarifmahfuz requested a review from a team May 10, 2024 22:49
Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leak should always be triggered with the same version of the code where it happened, but I see no reason not to more gracefully handle this mistake.

ruby/lib/ci/queue/bisect.rb Outdated Show resolved Hide resolved
@zarifmahfuz zarifmahfuz merged commit 5c9bc77 into main May 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants