Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minitest: avoid installing at_exit #236

Merged
merged 5 commits into from
Jan 9, 2024
Merged

Minitest: avoid installing at_exit #236

merged 5 commits into from
Jan 9, 2024

Conversation

3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Jan 8, 2024

Rails in test/test_helper.rb includes:

require 'rails/test_help'

that contains:

require "active_support/testing/autorun"

which calls Minitest.autorun, which, in turn, installs an at_exit that will run Minitest tests.

This is useful when you don't want to call Minitest.run explicitly. But we actually do so in the Knapsack Pro runner.

We forgot to add the class_variable_set as part of 6849d1f (make sure to read the link to the Minitest issue in the commit body).

This change is not needed in Regular Mode because we don't call Minitest.run (and we rely on the autorun) in that case.

Reproduction steps

  • Remove ::Minitest.class_variable_set('@@installed_at_exit', true)
  • cd rails-app-with-knapsack_pro
  • bin/knapsack_pro_queue_minitest
  • Notice an empty batch is run after Knapsack Pro executed all the batches

You can also see it on CI (notice the last few lines):

Finished in 0.623779s, 4.8094 runs/s, 4.8094 assertions/s.
3 runs, 3 assertions, 0 failures, 0 errors, 1 skips
----------After Subset Queue Hook - run after subset of test suite----------D, [2024-01-05T17:10:27.969366 #637] DEBUG -- : [knapsack_pro] POST https://api-staging.knapsackpro.com/v1/queues/queue
D, [2024-01-05T17:10:27.969421 #637] DEBUG -- : [knapsack_pro] API request UUID: 65453812-2682-4694-a544-c5deedc1eceb
D, [2024-01-05T17:10:27.969445 #637] DEBUG -- : [knapsack_pro] API response:
D, [2024-01-05T17:10:27.969474 #637] DEBUG -- : [knapsack_pro] {"queue_name"=>"125:4af5db3e6e2e3c36fcce7b1ca8cae41a", "build_subset_id"=>nil, "test_files"=>[]}
----------After Queue Hook - run after test suite----------D, [2024-01-05T17:10:28.201182 #637] DEBUG -- : [knapsack_pro] POST https://api-staging.knapsackpro.com/v1/build_subsets
D, [2024-01-05T17:10:28.201251 #637] DEBUG -- : [knapsack_pro] API request UUID: b30c32db-1ea9-418c-b5c3-ea6b672b2846
D, [2024-01-05T17:10:28.201276 #637] DEBUG -- : [knapsack_pro] API response:
D, [2024-01-05T17:10:28.201299 #637] DEBUG -- : [knapsack_pro] 
D, [2024-01-05T17:10:28.201329 #637] DEBUG -- : [knapsack_pro] Saved time execution report on Knapsack Pro API server.
Run options: --seed 30553

# Running:



Finished in 0.001104s, 0.0000 runs/s, 0.0000 assertions/s.
0 runs, 0 assertions, 0 failures, 0 errors, 0 skips
D, [2024-01-05T17:10:28.204810 #637] DEBUG -- : [knapsack_pro] Global time execution for tests: 0.6018200909984444s

@3v0k4 3v0k4 requested a review from ArturT January 8, 2024 14:44
@3v0k4 3v0k4 marked this pull request as ready for review January 8, 2024 15:20
@ArturT ArturT added the bug label Jan 8, 2024
Copy link
Member

@ArturT ArturT left a comment

Choose a reason for hiding this comment

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

Good catch!

@@ -211,6 +216,20 @@ jobs:
export KNAPSACK_PRO_TEST_DIR=turnip
export KNAPSACK_PRO_TEST_FILE_PATTERN="turnip/**/*.feature"
bundle exec rake "knapsack_pro:queue:rspec[-r turnip/rspec]"
- run:
Copy link
Member

Choose a reason for hiding this comment

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

You are adding Minitest steps to the jobs that are supposed to be testing the matrix of Ruby & RSpec versions.

I think we could make it cleaner. You could add new jobs to the workflow for testing the matrix of Ruby & Minitest versions. Thanks to that:

  • we have the option to test different Minitest versions in the future.
  • we would avoid testing the matrix of different Minitest & RSpec versions because it does not make sense. The testing frameworks are independent.

You could create a new story for that to not hold off on the release of this fix.

workflows:
  tests:
    jobs:
      - unit
      - integration-regular-rspec:
          name: integration-regular__ruby-<< matrix.ruby >>__rspec-<< matrix.rspec >>
          matrix:
            parameters:
              ruby: ["3.0", "3.1", "3.2"]
              rspec: ["3.10.2", "3.11.0", "3.12.2"]
      - integration-queue-rspec:
          name: integration-queue__ruby-<< matrix.ruby >>__rspec-<< matrix.rspec >>
          matrix:
            parameters:
              ruby: ["3.0", "3.1", "3.2"]
              rspec: ["3.10.2", "3.11.0", "3.12.2"]
      - integration-regular-minitest:
          name: integration-regular__ruby-<< matrix.ruby >>__minitest-<< matrix.minitest >>
          matrix:
            parameters:
              ruby: ["3.0", "3.1", "3.2"]
              minitest: ["x.x.x", ...]
      - integration-queue-minitest:
          name: integration-queue__ruby-<< matrix.ruby >>__minitest-<< matrix.minitest >>
          matrix:
            parameters:
              ruby: ["3.0", "3.1", "3.2"]
              minitest: ["x.x.x", ...]

Copy link
Contributor Author

@3v0k4 3v0k4 Jan 9, 2024

Choose a reason for hiding this comment

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

✅ (I had to reduce the parallelism to avoid jobs being queued)

@3v0k4 3v0k4 merged commit 3c953d1 into master Jan 9, 2024
25 checks passed
@3v0k4 3v0k4 deleted the minitest branch January 9, 2024 14:29
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