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-7722] Deduplicate dynamically generated RSpec examples using metadata.scoped_id #101

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

anmarchenko
Copy link
Contributor

@anmarchenko anmarchenko commented Jan 11, 2024

What does this PR do?
Adds deduplication for RSpec examples using "test.parameters.metadata" and "scoped_id" provided by rspec which is a stable label assigned to each example:

image

Motivation
Shared examples and dynamically generated examples with the same name were having the same fingerprint in Datadog test runs view.

Why does it work like that?
RSpec has many ways to define test with parameters.

  1. Using shared examples (source):
  shared_examples 'literal interpolation' do |literal, expected = literal|
    it "registers an offense for #{literal} in interpolation " \
       'and removes interpolation around it' do
      expect_offense(<<~'RUBY', literal: literal)
        "this is the #{%{literal}}"
                       ^{literal} Literal interpolation detected.
      RUBY

      expect_correction(<<~RUBY)
        "this is the #{expected}"
      RUBY
    end

  it_behaves_like('literal interpolation in words literal', '%W')
  it_behaves_like('literal interpolation in words literal', '%I')
  1. Dynamically generating examples with the same name in place (source):
    %w[| ^ & <=> == === =~ > >= < <= << >> + - * / % ** ~ ! != !~ && ||].each do |operator|
      it 'registers and corrects an offense with ternary operator and adding parentheses ' \
         'when if/then/else/end is preceded by an operator' do
        expect_offense(<<~RUBY, operator: operator)
          a %{operator} if cond then run else dont end
            _{operator} ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{if_offense_message}
        RUBY

        expect_correction(<<~RUBY)
          a #{operator} (cond ? run : dont)
        RUBY
      end
    end

In both cases the creation of examples happens on code loading stage before the test is executed which is trickier and more error-prone to instrument. Conveniently, for this case rspec provides metadata[:scoped_id] field that uniquely identifies example in example group hierarchy (test in test suite hierarchy using Datadog terms). I decided to use this field in metadata key under test.parameters to make it part of fingerprint for all tests thus ensuring their uniqueness without making test names less readable by including obscure ids in them.

@anmarchenko anmarchenko changed the title [CIVIS-7722] Add parametrized tests support to RSpec [CIVIS-7722] Deduplicate dynamically generated RSpec examples using metadata.scoped_id Jan 12, 2024
@anmarchenko anmarchenko marked this pull request as ready for review January 12, 2024 15:34
@anmarchenko anmarchenko requested review from a team as code owners January 12, 2024 15:34
@@ -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]})

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.

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.

@@ -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]})

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.

@anmarchenko anmarchenko merged commit c56656c into main Jan 15, 2024
26 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/parametrized_rspec_tests branch January 15, 2024 10:05
@github-actions github-actions bot added this to the 0.7.0 milestone Jan 15, 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.

None yet

2 participants