Skip to content

Skip loading a test file path for Minitest in Queue Mode when it does not exist on the disk#174

Merged
ArturT merged 3 commits intomasterfrom
skip-loading-non-existing-test-file
Jul 19, 2022
Merged

Skip loading a test file path for Minitest in Queue Mode when it does not exist on the disk#174
ArturT merged 3 commits intomasterfrom
skip-loading-non-existing-test-file

Conversation

@ArturT
Copy link
Copy Markdown
Member

@ArturT ArturT commented Jul 19, 2022

problem

A user reported an issue.

How to reproduce it:

  • you are using Queue Mode and Minitest and flag KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true
  • you run CI build for a new git commit. In the knapsack pro dashboard the user notices that there is path /usr/local/bundle/gems/minitest-5.16.2/lib/minitest/spec.rb recorded. It does not belong to the test suite.
  • when the user retried CI build (for instance, on Buildkite) then Knapsack Pro API returns a list of cached test file paths executed during the previous CI build. It means the API returned the path /usr/local/bundle/gems/minitest-5.16.2/lib/minitest/spec.rb.
  • knapsack_pro gem converts /usr/local/bundle/gems/minitest-5.16.2/lib/minitest/spec.rb path to relative path by adding ./ as prefix and attempts to load it but the /usr/local/bundle/gems/minitest-5.16.2/lib/minitest/spec.rb test file path does not exist on the disk. We should ignore the /usr/local/bundle/gems/minitest-5.16.2/lib/minitest/spec.rb test file path because it's not part of the test suite and was wrongly recorded.

changes

Skip loading a test file path for Minitest in Queue Mode when it does not exist on the disk.

Most likely, the test file path should not be loaded. The test file path could have been recorded during the previous CI build when the knapsack_pro gem could not attribute the execution time of a test to a correct test file path. For instance, you have shared examples in your test suite, and the knapsack_pro gem could not correctly determine for which test file path they were executed. In such a case, the test file path should not be loaded because the actual test cases will be executed by loading a correct test file path (which automatically happens because we recorded all test file paths existing on the disk during the previous CI build even with 0 seconds of execution time).

possible future improvements

It would be better to improve how we attribute test execution time to a correct test file path in MinitestAdapter.test_path method

but it's not that simple for shared examples. For shared examples, we are getting a path to the location of shared examples instead of the location where the shared examples are included.

Skip loading a test file path for Minitest in Queue Mode when it does not exist on the disk.

Most likely, the test file path should not be loaded. The test file path could have been recorded during the previous CI build when the knapsack_pro gem could not attribute the execution time of a test to a correct test file path. For instance, you have shared examples in your test suite, and the knapsack_pro gem could not correctly determine for which test file path they were executed. In such a case, the test file path should not be loaded because the actual test cases will be executed by loading a correct test file path. You can ignore this warning.
@ArturT ArturT added the bug label Jul 19, 2022
@ArturT ArturT requested a review from shadre July 19, 2022 16:44
@ArturT ArturT merged commit 80055b7 into master Jul 19, 2022
@ArturT ArturT deleted the skip-loading-non-existing-test-file branch July 19, 2022 16:57
Copy link
Copy Markdown
Member

@shadre shadre left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants