fix: Test Manifest, parameterized do not override sibling failures - #15831
Merged
josevalim merged 4 commits intoSep 2, 2026
Merged
Conversation
Fixes elixir-lang#15820 Alternative approach to elixir-lang#15825. In short, parameterized tests aen't accounted for in the test manifest, so if a variant first failed it could get overridden by a later run with different parameters as the key is just {module, test}. Based on [feedback](elixir-lang#15825 (comment)), this PR tries to solve this by keeping _all_ parameterized tests marked as failing. So for variants 1, 2, 3, 4 ran and only 2 failed, all 4 get rerun which isn't too much of a cost. For this small cost, we can keep the format of the manifest file, as it was before. There is one wrinkle to this, which is I couldn't find a good way to stop the override ("do not delete a failing test from the manifest, if I have the same id") while still ever deleting a failing test (which we need, otherwise `mix test --failed` would be ever growing) other than to track which tests failed _this run_. So, we only refuse to clear it if we know the test failed _this run_. The additional work to track is quite minimal though. It adds an additional argument to `put_test` though, which I decided to give a default to not increase the splash radius of the PR. Worth potentially removing, unless we think it's fair game for external callers.
Member
|
@PragTob thank you! I slightly changed the code to keep the passed and failures separately in the manifest. I also removed the integration test since the unit test gives us enough coverage! |
Member
|
💚 💙 💜 💛 ❤️ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15820
Alternative approach to #15825.
In short, parameterized tests aen't accounted for in the test manifest, so if a variant first failed it could get overridden by a later run with different parameters as the key is just {module, test}.
Based on feedback, this PR tries to solve this by keeping all parameterized tests marked as failing.
So for variants 1, 2, 3, 4 ran and only 2 failed, all 4 get rerun which isn't too much of a cost.
For this small cost, we can keep the format of the manifest file, as it was before.
There is one wrinkle to this, which is I couldn't find a good way to stop the override ("do not delete a failing test from the manifest, if I have the same id") while still ever deleting a failing test (which we need, otherwise
mix test --failedwould be ever growing) other than to track which tests failed this run. So, we only refuse to clear it if we know the test failed this run.The additional work to track is quite minimal though.
It adds an additional argument to
put_testthough, which I decided to give a default to not increase the splash radius of the PR. Worth potentially removing, unless we think it's fair game for external callers.