Allow TEST_ENV_NUMBER to support duplicate file groups #943

jaydorsey · 2024-03-30T21:43:31Z

This is a follow up to #940

After that PR was merged, I was writing a little helper script that basically let me do <script> <file> and it would basically duplicate that file name sysctl -n hw.ncpu, and then run parallel_rspec w/ the --allow-duplicates flag.

What I discovered was that the TEST_ENV_NUMBER was set to the same value for each file. I traced the root behavior back to the small change I made in this PR

Checklist

Feature branch is up-to-date with master (if not - rebase it).
Added tests
Added an entry to the Changelog if the new
code introduces user-observable changes.
Update Readme.md when cli options are changed

jaydorsey · 2024-03-30T22:10:49Z

lib/parallel_tests/cli.rb

+        if options[:allow_duplicates]
+          proc { start += 1 }
+        else
+          ->(groups, group) { groups.index(group) }


This is the original behavior. As I was writing this, I was wondering:

Could we use the proc/counter as a default for everything?

Is there something else I'm missing like "spec order randomization" where it's not as simple as just using an incrementing counter (e.g. maybe we don't even need the proc; we could just increment)

If it's not as simple as this proc (or a basic counter) I'm open to other suggestions; the only other idea I had was to track the index in a hash with a counter so I could see when I've re-used a group already.

If none of these ideas would work, then maybe --allow-duplicates should only work with a single file? This would be OK for my primary use case (and still useful I think)

spec/parallel_tests/cli_spec.rb

lib/parallel_tests/cli.rb

grosser

hmm so I think the issue is that the groups all look the same to the code since we use index
so maybe instead of the current code we could iterate the groups with each_with_index and get our index that way ?

jaydorsey · 2024-03-31T17:10:33Z

hmm so I think the issue is that the groups all look the same to the code since we use index so maybe instead of the current code we could iterate the groups with each_with_index and get our index that way?

Yeah that's a good point. I wasn't 100% confident that I understood what was going on inside of the execute_in_parallel block so I went down the rabbit hole of trying to shim in some new behavior, conditionally.

I just pushed up another branch that uses Parallel#map_with_index that looks right in my head, but I'm also having trouble visualizing/tracing/wrapping my head around the block yields.

I need to test it on my other machine to make triple-sure it's doing what I think it's doing (will do that later today)

jaydorsey · 2024-03-31T17:13:38Z

lib/parallel_tests/cli.rb

@@ -57,8 +57,8 @@ def execute_in_parallel(items, num_processes, options)
      Tempfile.open 'parallel_tests-lock' do |lock|
        ParallelTests.with_pid_file do
          simulate_output_for_ci options[:serialize_stdout] do
-            Parallel.map(items, in_threads: num_processes) do |item|
-              result = yield(item)
+            Parallel.map_with_index(items, in_threads: num_processes) do |item, index|


If I understand all of this correctly, the item that gets yielded on the next line is the group in the execute_in_parallel call on line 84

If that's the case, I wonder if naming these to groups/group instead of items/item makes it easier to understand?

execute_in_parallel is a generic method that does not always get groups passed
for example execute_in_parallel(runs, runs.size, options)
so afaik this should stay items

grosser · 2024-03-31T18:15:01Z

lib/parallel_tests/cli.rb

-            Parallel.map(items, in_threads: num_processes) do |item|
-              result = yield(item)
+            Parallel.map_with_index(items, in_threads: num_processes) do |item, index|
+              result = yield(item, index)


cli.rb:376 will need a change to accept the index

... hmm I guess no it does not since it ignores the extra arg 👍

grosser · 2024-03-31T18:16:41Z

lib/parallel_tests/cli.rb

-        test_results = execute_in_parallel(groups, groups.size, options) do |group|
-          run_tests(group, groups.index(group), num_processes, options)
+        test_results = execute_in_parallel(groups, groups.size, options) do |group, index|
+          test_env_number = options[:first_is_1] ? index + 1 : index


the first_is_1 logic was not here before, so it should get removed or be removed from some other place ?

You're right, good catch. I didn't trace the test_env down far enough; I see it in test/runner.rb now.

I've updated the PR to remove line 85 and just use index.

jaydorsey · 2024-04-01T14:21:15Z

I've been testing this with a little helper script I wrote to help me run 1 test multiple times in parallel:

(Edit: I've made this part of my dotfiles, with some tweaks here)

#!/usr/bin/env ruby
# frozen_string_literal: true

# Useful for running a single test multiple times, locally
#
# 1. Save this file locally as something like `bin/runit`, or put it in your path
# 2. chmod +x bin/runit
# 3. Add it to your gitignore and/or gitignore_global file
#
# Usage:
#
#    bin/runit spec/models/user_spec.rb
cpus  = (`sysctl -n hw.ncpu`.to_i - 1)
files = "#{ARGV[0]} " * cpus
exec("bin/parallel_rspec --allow-duplicates --verbose-command --prefix-output-with-test-env-number -- #{files}")

I've tested it with and without the first-is-1 flag and it all runs exactly like I expect:

Test numbers appear correct w/ and w/out the flag
Zero database conflicts (which it had before) because it's using the correct/distinct envs for the same file now

(I get a lot of activerecord errors before, because it was using the same test env number/database)

Before

All the same test env (not intended/desired behavior):

These would happen pretty often:

After

With this PR, test envs all look correct

grosser · 2024-04-03T15:51:38Z

CHANGELOG.md

@@ -7,6 +7,7 @@
 ### Added

 ### Fixed
+- The `--allow-attributes` flag now runs duplicate tests in different groups


Suggested change

- The `--allow-attributes` flag now runs duplicate tests in different groups

- The `--allow-duplicates ` flag now runs duplicate tests in different groups

grosser

❤️ ended up being a great small bugfix :D

grosser · 2024-04-03T15:55:42Z

4.6.1

jaydorsey added 3 commits March 29, 2024 11:17

WIP: Allow for different grouping strategy

22c9aeb

Refactor cli.rb

5293cff

Test refactor

470801e

jaydorsey commented Mar 30, 2024

View reviewed changes

jaydorsey marked this pull request as ready for review March 30, 2024 22:13

grosser reviewed Mar 30, 2024

View reviewed changes

lib/parallel_tests/cli.rb Outdated Show resolved Hide resolved

grosser reviewed Mar 30, 2024

View reviewed changes

Use map_with_index

5ed8475

jaydorsey commented Mar 31, 2024

View reviewed changes

grosser reviewed Mar 31, 2024

View reviewed changes

Remove first_is_1 check

fa326ef

jaydorsey force-pushed the jaydorsey/groups branch from dbbf2e3 to fa326ef Compare April 1, 2024 14:08

Update changelog

034cc49

jaydorsey force-pushed the jaydorsey/groups branch from 16a502f to 034cc49 Compare April 3, 2024 13:56

grosser reviewed Apr 3, 2024

View reviewed changes

grosser approved these changes Apr 3, 2024

View reviewed changes

grosser merged commit 2ae6606 into grosser:master Apr 3, 2024
13 checks passed

jaydorsey deleted the jaydorsey/groups branch April 4, 2024 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow TEST_ENV_NUMBER to support duplicate file groups #943

Allow TEST_ENV_NUMBER to support duplicate file groups #943

jaydorsey commented Mar 30, 2024 •

edited

jaydorsey Mar 30, 2024

grosser left a comment

jaydorsey commented Mar 31, 2024

jaydorsey Mar 31, 2024

grosser Mar 31, 2024

grosser Mar 31, 2024

grosser Mar 31, 2024

grosser Mar 31, 2024

jaydorsey Apr 1, 2024

jaydorsey commented Apr 1, 2024 •

edited

grosser Apr 3, 2024 •

edited

grosser left a comment

grosser commented Apr 3, 2024

	- The `--allow-attributes` flag now runs duplicate tests in different groups
	- The `--allow-duplicates ` flag now runs duplicate tests in different groups

Allow TEST_ENV_NUMBER to support duplicate file groups #943

Allow TEST_ENV_NUMBER to support duplicate file groups #943

Conversation

jaydorsey commented Mar 30, 2024 • edited

Checklist

jaydorsey Mar 30, 2024

Choose a reason for hiding this comment

grosser left a comment

Choose a reason for hiding this comment

jaydorsey commented Mar 31, 2024

jaydorsey Mar 31, 2024

Choose a reason for hiding this comment

grosser Mar 31, 2024

Choose a reason for hiding this comment

grosser Mar 31, 2024

Choose a reason for hiding this comment

grosser Mar 31, 2024

Choose a reason for hiding this comment

grosser Mar 31, 2024

Choose a reason for hiding this comment

jaydorsey Apr 1, 2024

Choose a reason for hiding this comment

jaydorsey commented Apr 1, 2024 • edited

Before

After

grosser Apr 3, 2024 • edited

Choose a reason for hiding this comment

grosser left a comment

Choose a reason for hiding this comment

grosser commented Apr 3, 2024

jaydorsey commented Mar 30, 2024 •

edited

jaydorsey commented Apr 1, 2024 •

edited

grosser Apr 3, 2024 •

edited