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: fix rails parallel test runner #115

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Jan 25, 2024

What does this PR do?
Fixes rails parallel test executor instrumentation that was broken since I added ci-queue support to minitest framework.

Why was this broken?
In the previous episode of "Datadog figures out how to instrument all possible ways one can use Minitest" we watched minitest-reporters interfering with datadog-ci plugin which caused me to remove plugin and instrument Minitest::CompositeReporter to mark start and end of test sessions.

But... (suspenseful music) there is a twist!
Here is the Minitest.run method (important parts only):

  def self.run args = []
    self.load_plugins unless args.delete("--no-plugins") || ENV["MT_NO_PLUGINS"]

    # ... code omitted for brevity ...

    self.init_plugins options # <------------ here we start session after merging this PR

    self.parallel_executor.start if parallel_executor.respond_to?(:start) # here Rails forks process when using parallel executor
    reporter.start # <---------- here we started session before this PR 
    begin
      __run reporter, options
    rescue Interrupt
      warn "Interrupted. Exiting..."
    end
    self.parallel_executor.shutdown

    reporter.report # <---------- here we end session

    reporter.passed?
  end

Unfortunately as you can see reporter.start is called after the self.parallel_executor.start. This is usually not that important, but not for Rails parallel executor, ladies and gentlemen! When #start is called, it forks processes copying memory and creating an independent process to run your tests.

This worked fine when we used plugin to start session: datadog-ci stores test session in global context that is copied on fork to all child processes. But now, if we start session after workers are forked, then the session is stored only in main test process and not available to workers that actually execute tests.

This PR fixes this issue by moving session start (again) to the init_plugins method but instead of using official Minitest plugin system we are instrumenting this method directly because it is the only way to make it work for all cases discovered so far.

How to test the change?
Manual testing via https://github.com/anmarchenko/openstreetmap-website is required (this is how the issue was discovered). I am happy to pair if anyone would like to see that it works.

This project will be added to Datadog's integration testing environment later.

Why don't you have unit tests for rails parallel executor?
It is extremely tricky to get any data out of forked processes in unit tests, I created a task to investigate it and try figure out a method of creating test environment suitable for unit tests.

… method as it is executed before start of parallel_executor and will propagate global context to forked rails workers
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9acd580) 99.04% compared to head (d255285) 99.04%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #115   +/-   ##
=======================================
  Coverage   99.04%   99.04%           
=======================================
  Files         151      152    +1     
  Lines        6676     6691   +15     
  Branches      296      296           
=======================================
+ Hits         6612     6627   +15     
  Misses         64       64           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anmarchenko anmarchenko marked this pull request as ready for review January 25, 2024 14:26
@anmarchenko anmarchenko requested review from a team as code owners January 25, 2024 14:26
@anmarchenko anmarchenko changed the title minitest: start test session and test module in Minitest#init_plugins method as it is executed before start of parallel_executor and will propagate global context to forked rails workers minitest: fix rails parallel test runner Jan 25, 2024
@anmarchenko anmarchenko merged commit 054512f into main Jan 25, 2024
27 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/fix_rails_test_runner branch January 25, 2024 15:15
@github-actions github-actions bot added this to the 0.7.0 milestone Jan 25, 2024
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.

3 participants