Skip to content

Fix compare mode to not check node bucket files#179

Open
JuanVqz wants to merge 5 commits intomainfrom
feature/compare-filter-own-buckets
Open

Fix compare mode to not check node bucket files#179
JuanVqz wants to merge 5 commits intomainfrom
feature/compare-filter-own-buckets

Conversation

@JuanVqz
Copy link
Copy Markdown
Member

@JuanVqz JuanVqz commented Apr 15, 2026

Summary

While replying to #98, I realized that compare mode in parallel setups should not check for node-specific shard files since those nodes do not have full test suite data, which would result in false positives.

  • node_index is now only needed for save mode (to write shard files)
  • For compare the node_index is ignore it always check against the final shitlist file.
  • Updated README to clarify that node_index is only for save mode
  • Added test to verify node_index is safely ignored in compare mode

JuanVqz added 5 commits April 15, 2026 16:49
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.
Ruby 2.3 parses '@mode == :compare ? nil : node_index' as
'@mode == (:compare ? nil : node_index)'. Adding explicit
parentheses fixes the precedence.
@JuanVqz JuanVqz self-assigned this Apr 15, 2026
@JuanVqz JuanVqz requested review from arielj and etagwerker April 15, 2026 23:02
@JuanVqz JuanVqz marked this pull request as ready for review April 15, 2026 23:02
@arielj
Copy link
Copy Markdown

arielj commented Apr 16, 2026

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?

@JuanVqz
Copy link
Copy Markdown
Member Author

JuanVqz commented Apr 16, 2026

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 JuanVqz changed the title Fix compare mode to only check buckets the current process ran Fix compare mode to not check node bucket files Apr 16, 2026
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.

2 participants