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.

Sign up for GitHub

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

Jump to bottom

[CIVIS-7722] Deduplicate dynamically generated RSpec examples using metadata.scoped_id #101

Merged
merged 2 commits into from
Jan 15, 2024
Merged

[CIVIS-7722] Deduplicate dynamically generated RSpec examples using metadata.scoped_id #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb7066a
add parametrized shared examples to our rspec instrumentation tests
anmarchenko Jan 11, 2024
76f2a35
exploring using RSpec scoped_id metadata as parameters metadata to ma…
anmarchenko Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/datadog/ci/contrib/rspec/example.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ def run(example_group_instance, reporter)
},
service: configuration[:service_name]
) do |test_span|
test_span.set_parameters({}, {"scoped_id" => metadata[:scoped_id]})
Copy link

Choose a reason for hiding this comment

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

Is :scoped_id comparable to an index (eg: if you change the values of the parameters, the first value will still have the same scoped_id)?

I'd be slightly worried about having different parameter values with the same scoped_id, in theory, but I imagine that'd be more or less impossible to do without involving some kind of resource not covered by code coverage (which is a problem that exists regardless of parameters).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if parameter value will change the scoped_id will stay the same.

So if you had tests like:

[1, 2, 3, 4, 5] do |number|
  it "prints number" do
    expect(print(number)).to be_truthy
  end
end

and then you changed them like that:

[1, 2, 4, 5] do |number|
  it "prints number" do
    expect(print(number)).to be_truthy
  end
end

Now test that tests parameter "4" has the same fingerprint as test that previously tested "3" and the test with "5" now the same that the test with "4" before.

The issue is that with dynamically generated code like that we don't even know that this example was created 5 times with different variables in scope, because there is no passing of argument to any method happening here per se.

I am 99% sure though that there could be a way to catch it in Ruby (because almost everything is possible if you try hard enough), but right now I am not convinced that we need to invest a lot of time into that right now.

There is an easy way for app developers to distinguish these tests based on parameter:

[1, 2, 4, 5] do |number|
  it "prints number #{number}" do
    expect(print(number)).to be_truthy
  end
end

Then we'll see tests "prints number 1", "prints number 2", etc in the dashboard.

Copy link

Choose a reason for hiding this comment

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

From ITR's perspective, I think that's okay (because you can't change the parameter value without modifying the file, so that'll mark it as modified).

I'm not sure what the impact is for some of our other tools (eg: flaky test detection, or some of the test history queries that power the UI), but I think you're likely right that going this route for now is the proper decision.


result = super

case execution_result.status
Expand Down
2 changes: 1 addition & 1 deletion lib/datadog/ci/span.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def set_default_tags
# @param [Hash] metadata optional metadata
# @return [void]
def set_parameters(arguments, metadata = {})
return if arguments.nil? || arguments.empty?
return if arguments.nil?

set_tag(
Ext::Test::TAG_PARAMETERS,
Expand Down
15 changes: 12 additions & 3 deletions spec/datadog/ci/contrib/rspec/instrumentation_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,8 @@ def rspec_session_run(with_failed_test: false, with_shared_test: false)

if with_shared_test
require_relative "some_shared_examples"
include_examples "Testing shared examples"
include_examples "Testing shared examples", 2
include_examples "Testing shared examples", 1
end
end

Expand Down Expand Up @@ -371,8 +372,16 @@ def rspec_session_run(with_failed_test: false, with_shared_test: false)
let!(:spec) { rspec_session_run(with_shared_test: true) }

it "creates correct test spans connects all tests to a single test suite" do
shared_test_span = test_spans.find { |test_span| test_span.name == "SomeTest shared examples adds 1 and 1" }
expect(shared_test_span.get_tag(Datadog::CI::Ext::Test::TAG_SUITE)).to eq(spec.file_path)
shared_test_spans = test_spans.filter { |test_span| test_span.name == "SomeTest shared examples adds 1 and 1" }
expect(shared_test_spans).to have(2).items

shared_test_spans.each_with_index do |shared_test_span, index|
expect(shared_test_span.get_tag(Datadog::CI::Ext::Test::TAG_SUITE)).to eq(spec.file_path)

expect(shared_test_span.get_tag(Datadog::CI::Ext::Test::TAG_PARAMETERS)).to eq(
"{\"arguments\":{},\"metadata\":{\"scoped_id\":\"1:#{2 + index}:1\"}}"
)
end

test_spans.each do |test_span|
expect(test_span.get_tag(Datadog::CI::Ext::Test::TAG_TEST_SUITE_ID)).to eq(test_suite_span.id.to_s)
Expand Down
4 changes: 2 additions & 2 deletions spec/datadog/ci/contrib/rspec/some_shared_examples.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
RSpec.shared_examples "Testing shared examples" do
RSpec.shared_examples "Testing shared examples" do |expected_result|
context "shared examples" do
it "adds 1 and 1" do
expect(1 + 1).to eq(2)
expect(1 + 1).to eq(expected_result)
end
end
end