-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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).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, if parameter value will change the
scoped_id
will stay the same.So if you had tests like:
and then you changed them like that:
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:
Then we'll see tests "prints number 1", "prints number 2", etc in the dashboard.
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.
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.