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

[CIVIS-10061] Fix Knapsack Pro integration #180

Merged
merged 5 commits into from
May 22, 2024

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented May 22, 2024

Fixes #147

This PR fixes the following issues with our current Knapsack Pro support:

Dropping tests because of missing test session

When running knapsack_pro:rspec:queue command, Datadog.configure block gets called when rails_helper is required which in this case happens after KnapsackPro::Extensions::RSpecExtension.setup! had been run.

To fix this, we check whether RSpec::Core::Runner was already instrumented by knapsack: if yes, we instrument RSpec::Core::Runner directly. If not, we hook into KnapsackPro::Extensions::RSpecExtension to instrument the runner after knapsack injects its methods:

if defined?(::KnapsackPro::Extensions::RSpecExtension::Runner) &&
  ::RSpec::Core::Runner.ancestors.include?(::KnapsackPro::Extensions::RSpecExtension::Runner)
  # knapsack already patched rspec runner
  require_relative "runner"
  ::RSpec::Core::Runner.include(KnapsackPro::Runner)
else
  # knapsack didn't patch rspec runner yet
  require_relative "extension"
  ::KnapsackPro::Extensions::RSpecExtension.include(KnapsackPro::Extension)
end

Noise from knapsack_pro:rspec_test_example_detector test command

We noticed that in many cases when using knapsack, we got a lot more test runs than there are unique tests; additional test runs all have very short duration and are part of knapsack_pro:rspec_test_example_detector.

It turned out, that we trace rspec session that is running with --dry-run: test example detector from knapsack uses dry run to generate a report with all rspec examples.

This issue is fixed by never tracing rspec tests when dry run is enabled:

return super if ::RSpec.configuration.dry_run?

How to test the change?
Unit tests are provided and additionally tested with Github Actions: https://github.com/DataDog/rails-app-with-knapsack_pro/actions/workflows/main.yaml

The test runs arrived correctly:
image

There are no additional test runs from rspec_test_example_detector - the number of tests is equal to the number of test runs (51 vs 51)

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.95%. Comparing base (80e7b13) to head (25bf96c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #180   +/-   ##
=======================================
  Coverage   98.94%   98.95%           
=======================================
  Files         205      209    +4     
  Lines        9688     9758   +70     
  Branches      445      452    +7     
=======================================
+ Hits         9586     9656   +70     
  Misses        102      102           

☔ 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 May 22, 2024 14:30
@anmarchenko anmarchenko requested review from a team as code owners May 22, 2024 14:30
@anmarchenko anmarchenko merged commit 1d12868 into main May 22, 2024
26 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/knapsack_pro_integration branch May 22, 2024 14:57
@github-actions github-actions bot added this to the 1.0.0 milestone May 22, 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.

Dropping tests with knapsack_pro
3 participants