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

Add specs that were not executed to test:contrib task #3855

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

y9v
Copy link
Member

@y9v y9v commented Aug 21, 2024

What does this PR do?
Some specs in spec/datadog/tracing/contrib folder were not executed with test:contrib rake task.
This commit enables them to see if they fail.

Motivation:
When working on adding http.route to tracing, I noticed a one file - spec/datadog/tracing/contrib/http_route_spec.rb that should have been failing in CI.

After doing a small investigation with help of @anmarchenko it was clear that this file is not being executed, along with some other spec files.

This is the result we got when running rake test:contrib with a breakpoint in RSpec::Core::World:

Dir.glob('./spec/datadog/tracing/contrib/*_spec.rb') - @example_groups.map(&:file_path)
=>
["./spec/datadog/tracing/contrib/component_spec.rb",
 "./spec/datadog/tracing/contrib/grpc_spec.rb",
 "./spec/datadog/tracing/contrib/http_route_spec.rb",
 "./spec/datadog/tracing/contrib/http_spec.rb",
 "./spec/datadog/tracing/contrib/span_attribute_schema_spec.rb",
 "./spec/datadog/tracing/contrib/status_range_env_parser_spec.rb",
 "./spec/datadog/tracing/contrib/status_range_matcher_spec.rb"]

Additional Notes:
Most likely some of added specs will fail, this is expected.

How to test the change?
It will be tested in CI pipeline.

@y9v y9v self-assigned this Aug 21, 2024
@y9v y9v requested a review from a team as a code owner August 21, 2024 12:44
@pr-commenter
Copy link

pr-commenter bot commented Aug 21, 2024

Benchmarks

Benchmark execution time: 2024-08-22 17:01:28

Comparing candidate commit 8c5928e in PR branch enable-lost-contrib-specs with baseline commit c5ab063 in branch master.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 20 metrics, 2 unstable metrics.

scenario:profiler - sample timeline=false

  • 🟥 throughput [-0.434op/s; -0.347op/s] or [-6.258%; -5.002%]

scenario:tracing - Propagation - Datadog

  • 🟥 throughput [-1227.539op/s; -1127.265op/s] or [-3.639%; -3.342%]

scenario:tracing - Propagation - Trace Context

  • 🟥 throughput [-1368.128op/s; -1248.984op/s] or [-3.556%; -3.246%]

@ivoanjo
Copy link
Member

ivoanjo commented Aug 21, 2024

Some specs in spec/datadog/tracing/contrib folder were not executed with test:contrib rake task.
This commit enables them to see if they fail.

Really nice find!

This PR is for opening a discussion - should we delete those files or fix them if they fail?

My suggestion on this one would be to add pendings with TODOs for individual specs that fail. That way we could incrementally fix/review such specs.

I'm spotting that CI is failing due to missing grape, so I think at least some of the tests need to be run under the grape appraisal?

Another question is whether it makes sense to change test:contrib task definition to execute all specs from contrib folder. Most likely there was some good reason to not do so.

I suspect it may be due to some of them needing extra dependencies.

Some specs in spec/datadog/tracing/contrib folder were not executed with
`rake test:contrib` rake task. This commit enables them to see if they
fail.
@y9v y9v force-pushed the enable-lost-contrib-specs branch from 4ff1b98 to 8c5928e Compare August 22, 2024 16:27
@y9v y9v requested a review from a team as a code owner August 22, 2024 16:27
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.85%. Comparing base (c5ab063) to head (8c5928e).
Report is 139 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3855      +/-   ##
==========================================
- Coverage   97.86%   97.85%   -0.02%     
==========================================
  Files        1271     1277       +6     
  Lines       75976    76255     +279     
  Branches     3739     3739              
==========================================
+ Hits        74356    74619     +263     
- Misses       1620     1636      +16     

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

@p-datadog
Copy link
Contributor

p-datadog commented Aug 23, 2024

What is the situation with the removed http route test? Is the corresponding code covered by other tests, is there a plan to reinstate the removed test? Is it covered by #3849 ?

@y9v
Copy link
Member Author

y9v commented Aug 26, 2024

@p-datadog this http route test was actually introduced in the first attempt of http.route implementation that was rolled back (due to incompatibility with rails 7.1). I am not exactly sure why this file still exists in the repo, maybe due to incorrect rebase.
I was not sure if I should add it to my PR, since I think we have no appraisals that includs grape, sinatra and rails. I will try to add it to my PR and see if I can make it pass.

Copy link
Member

@anmarchenko anmarchenko left a comment

Choose a reason for hiding this comment

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

I propose to merge this now, it executes more tests than before anyway.

'*' in contrib_paths works better than what we had before: now newly added tests will be executed by default, not ignored by default as before. Let's handle http route testing in its own PR. Thanks for handling this, @y9v

@y9v y9v merged commit c515a5e into master Aug 26, 2024
187 checks passed
@y9v y9v deleted the enable-lost-contrib-specs branch August 26, 2024 09:12
@github-actions github-actions bot added this to the 2.4.0 milestone Aug 26, 2024
@p-datadog
Copy link
Contributor

👍

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.

5 participants