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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

On RSpec integration, change suite name to example group file path (i.e. "fix" shared_examples suite names) #1816

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

Drowze
Copy link
Contributor

@Drowze Drowze commented Dec 23, 2021

Hello DataDog crew 馃憢
As my first PR here, please let me know if there's anything weird on the PR (diffs or description) 馃槃

The problem

In Datadog we report all tests' suite names as the example file path, which usually is the same as the example group file path. This is not always true however, specifically for the case of tests included from a different path (usually on a shared_examples block).

Because of that, suite name would be reported as the shared examples file path, which was not not very helpful to track the files that are taking the most to run as the shared examples can be on a different path than the test file that included them.

For example, we have some test examples on a "vehicle_controller_example.rb" file that are included a few times on some other controller and, instead of that controller appearing as slow suite on DataDog, we have the shared example file as such, see:
image

The solution

To address this, I'm adding the use of metadata[:example_group][:file_path] instead of simply file_path, so all test suites should be reported as the example group file path (meaning that, instead of the shared examples path, I'd see the controller path on the screenshot above).

@Drowze Drowze requested a review from a team December 23, 2021 12:07
@Drowze Drowze force-pushed the rg-example-path-on-shared-examples branch from b51eb1d to 04ffb01 Compare December 23, 2021 12:20
@delner delner added bug Involves a bug ci-app CI product for test suite instrumentation community Was opened by a community member labels Jan 4, 2022
Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed problem description! It really helped me understand the issue.

Overall the change makes sense to me; test is pretty straightforward. Should be fine.

Just as a heads up, it might be a bit till this rolls out, as we're trying to finalize a 1.0 release. We'll see how this fits in given the current schedule.

Thanks for the contribution!

@Drowze
Copy link
Contributor Author

Drowze commented Mar 14, 2022

Hey @delner

Any updates on the merging status of this PR? 馃槃

@delner
Copy link
Contributor

delner commented Mar 22, 2022

@Drowze Yes, I think we can merge this if all is well. Can you rebase this on the latest master? If so we can roll it into the next release.

In Datadog we report all tests' suite names as the example file path,
which usually is the same as the example group file path. This is not
always true however, especifically on the case of tests included from a
different path (usually on a shared_examples block).

Because of that, suite name would be reported as the shared examples
file path, which was not not very helpful to track the files that are
taking the most to run as the shared examples can be on a different
path than the test file that included them.

To address this, the this commit adds the use of
`metadata[:example_group][:file_path]`, so all test suites should be
reported as the _example group file path_.
@Drowze Drowze force-pushed the rg-example-path-on-shared-examples branch from 04ffb01 to fca42c7 Compare April 8, 2022 08:19
@Drowze
Copy link
Contributor Author

Drowze commented Apr 8, 2022

Sorry for the delay @delner

Just rebased. Hope it's not too late 馃槃

@delner delner added this to In review in Active work Apr 13, 2022
@delner delner added this to the 1.0.0.beta2 milestone Apr 13, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #1816 (fca42c7) into master (2ed82eb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1816   +/-   ##
=======================================
  Coverage   97.69%   97.69%           
=======================================
  Files        1000     1001    +1     
  Lines       50445    50453    +8     
=======================================
+ Hits        49283    49292    +9     
+ Misses       1162     1161    -1     
Impacted Files Coverage 螖
lib/datadog/ci/contrib/rspec/example.rb 96.96% <100.00%> (酶)
...c/datadog/ci/contrib/rspec/instrumentation_spec.rb 100.00% <100.00%> (酶)
...c/datadog/ci/contrib/rspec/some_shared_examples.rb 100.00% <100.00%> (酶)
spec/datadog/profiling/ext/forking_spec.rb 100.00% <0.00%> (+0.60%) 猬嗭笍

馃摚 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@delner delner left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@delner delner merged commit 2238cd2 into DataDog:master Apr 13, 2022
Active work automation moved this from In review to Merged & awaiting release Apr 13, 2022
@delner
Copy link
Contributor

delner commented Apr 15, 2022

This was released in 1.0.0.beta2.

@delner delner moved this from Merged & awaiting release to Released in Active work Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug ci-app CI product for test suite instrumentation community Was opened by a community member
Projects
Active work
  
Released
Development

Successfully merging this pull request may close these issues.

None yet

3 participants