-
Notifications
You must be signed in to change notification settings - Fork 5
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-7948] Test suite level visibility instrumentation for Minitest framework #92
Conversation
… start/end events
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
==========================================
+ Coverage 99.18% 99.22% +0.04%
==========================================
Files 139 142 +3
Lines 5624 5924 +300
Branches 216 233 +17
==========================================
+ Hits 5578 5878 +300
Misses 46 46 ☔ View full report in Codecov by Sentry. |
3e635ac
to
59c0ebd
Compare
…pec and parallel execution of test cases
…ate faster test suites
test_suite_name = Suite.name(self, method) | ||
|
||
test_suite = Datadog::CI.start_test_suite(test_suite_name) | ||
test_suite.passed! # will be overridden if any test fails |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it mean a test suite can never have 'skipped' status? Not sure what the intended behaviour is, mentioning it just in case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I haven't implemented SKIP
for test suites in any of the instrumentations right now. Is it heavily used by other libraries (if we don't include ITR)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Java the suite status is SKIP if it contains 0 test cases, or if every test case in it is skipped. Not sure about other languages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you! I will add a note for the future improvements
module Suite | ||
def self.name(klass, method_name) | ||
source_location, = klass.instance_method(method_name).source_location | ||
source_file_path = Pathname.new(source_location.to_s).relative_path_from(Pathname.pwd).to_s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's implicitly assumed here that the current dir is always the same as the repo root. Not sure if the two can be different in Ruby world. If they can, things like source code integration and ITR will break in the backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't yet encountered cases where this wouldn't be true for Ruby as both bin/rails
and rake
command are always executed from the root. Note that this is actually a test suite name (which could be any arbitrary string), not the source code integration (which I will add together with ITR). For source code integration, I will check for the repository root.
…test::Parallel::Test in ancestors chain to detect parallelized Runnable
What does this PR do?
Adds test suite level visibility instrumentation for Minitest framework
Important design note
The most important decision made during the development was about the concurrent test execution in Minitest.
When tests are executed serially, they are executed suite by suite (or Runnable by Runnable in Minitest case) just like in RSpec or Cucumber. Things change when tests are executed concurrently (which is a built in feature for Minitest). In this case runners (implemented as threads or forked processes) receive jobs and execute them one by one. "Job" in this case is a single test (or Runnable's method) not the whole runnable:
https://github.com/minitest/minitest/blob/master/lib/minitest/parallel.rb#L33
The proposed implementation is a pragmatic solution to this problem:
This produces the following session traces
For serial execution:
For concurrent execution:
This approach does not allow to track suite performance in concurrent mode but it shows the execution model better:
with concurrent execution enabled the most important performance metric is time to execute individual tests, not test files.
Other Notes
How to test the change?
Tested using the following repositories:
https://github.com/anmarchenko/openstreetmap-website
https://github.com/anmarchenko/sidekiq
https://github.com/anmarchenko/feedbin
https://github.com/anmarchenko/devdocs
https://github.com/anmarchenko/middleman