Fix compare mode to not check node bucket files#179
Conversation
Compare mode now always filters to buckets the current process actually ran, regardless of whether node_index is set. This fixes parallel test support (e.g. parallel_tests gem) where each process only sees a subset of specs but compares against the full shitlist. Previously, this filtering was gated behind the parallel? check which required node_index. Now node_index is only needed for save mode (to write shard files).
The filter was a no-op: buckets not run by this process have identical values in both normalized and stored (both read from the same shitlist file), so they always compare equal.
When node_index is set during compare, target_path would return the shard path (which does not exist after merge). This caused normalized_deprecation_messages to read an empty file instead of the full shitlist, breaking the comparison for non-run buckets.
|
I'm curious, does it even make sense to use compare for a shard? what I mean is that if you add a new test file, maybe your files tested in each shard change, rendering the compare kinda useless cause you won't be comparing the same files, or if using the timing function in CircleCI to split tests, if tests are added/removed that will change the times and maybe also change the files executed in each shard I feel like compare should be done at the end of the process after calling the merge command and not for a single shard? |
@arielj correct, this is the fix to follow that, compare should be run at the end (we cannot enforce that, can we?), what we can is ignore the node files even if the node_index is added, the compare command is only going to use the “general” shitlist file. Let me double check the changes 🤔 |
JuanVqz
changed the title
I agree with this. Would it make sense to not support compare if there is a Line 155 in 18586aa |
Prevents node_index in compare mode with error. Compare should only run on final merged shitlist, not on individual shards—shard-level compare is misleading because adding/removing tests changes file distribution. Co-Authored-By: Ariel Juodziulis <ariel@ombulabs.com> Co-Authored-By: Ernesto Tagwerker <ernesto+github@ombulabs.com>
etagwerker
left a comment
etagwerker
left a comment
There was a problem hiding this comment.
@JuanVqz Looks good, thanks! 👍🏻
3 participants
Summary
Compare mode should only run on the final merged shitlist, not on individual shard files. As Ariel noted, shard-level compare is misleading because adding/removing tests changes file distribution across shards.
This PR enforces that pattern by:
node_indexwith compare mode (raises error)node_indexis only for save mode (writing shard files)node_indexis save-mode onlyThis forces correct usage: compare only runs after merge command on final results.