Add helpful warning message for shared example groups #2278
Conversation
| old_definition = formatted_location existing_module.definition | ||
| new_definition = formatted_location new_block | ||
|
|
||
| if old_definition == new_definition |
There was a problem hiding this comment.
I think that old_definition == new_definition is a poor way to detect this. There are other reasons why old_definition == new_definition could evaluate to true (e.g. the user is using load instead of require to load the file from two places). It would be a lot more precise to see if the file is included in RSpec.configuration.loaded_spec_files. If so, then we know that the file is named in such a way to match the pattern and be loaded by RSpec itself, and there's no need to use qualifying language like "you might be seeing this warning because of a naming issue" or "if you have named the file to end in _spec.rb`.
There was a problem hiding this comment.
Thanks for the feedback! I've addressed your comment and hopefully I did something along the lines of what you were expecting. I figured it was still important that the definition of the shared example group to be on the same line in the same file, but I've added a check to ensure that the file was loaded by RSpec as well so we now know that's the root cause of the issue. I'd love your feedback on my instructions for resolution in the warning message, though. I think they're clear, but they may not be the best instructions for the user, so your input would be much appreciated.
|
Can you rebase once you've taken a look at @myronmarston's feedback? Build should be back to green now. |
devonestes
force-pushed
the
adding-error-message
branch
from
b191e89 to
455d6d1
Compare
| new_definition = formatted_location new_block | ||
| loaded_spec_files = RSpec.configuration.loaded_spec_files | ||
|
|
||
| if loaded_spec_files.include?(new_definition) && old_definition == new_definition |
There was a problem hiding this comment.
You don't need to do the == check at all, we already know this is a duplicate definition, we also don't know which definition is the wrong one so this should be:
loaded_spec_files.include?(new_definition) || loaded_spec_files.include?(old_definition)
There was a problem hiding this comment.
What we're checking for here isn't that it's a duplicate definition, actually, but a single definition of a shared example group that's loaded twice, which was saying that there was a duplicate definition when there wasn't really a duplication. For example, here's the warning I was seeing in my tests:
WARNING: Shared example group 'a flagger_or_tagger' has been previously defined
at:
/Users/devoncestes/esh/IRT/spec/services/shared_examples/flagger_or_tagger_spec.rb:10
...and you are now defining it at:
/Users/devoncestes/esh/IRT/spec/services/shared_examples/flagger_or_tagger_spec.rb:10
The new definition will overwrite the original one.
I didn't have a duplicate definition there, but because of the naming of the file combined with the explicit require that we were doing, it was evaluating that shared example group definition twice even though it was only defined once.
There was a problem hiding this comment.
What we're checking for here isn't that it's a duplicate definition, actually, but a single definition of a shared example group that's loaded twice, which was saying that there was a duplicate definition when there wasn't really a duplication.
It's already a duplicate definition, or it wouldn't trigger this method.
There was a problem hiding this comment.
It's already a duplicate definition, or it wouldn't trigger this method.
It's already a duplicate in terms of re-using the same name, but it is not necessarily being defined twice at the same location, which is what @devonestes's check is about.
There was a problem hiding this comment.
@devonestes -- I think it would help avoid this confusion if you renamed old_definition to old_definition_location and new_definition to new_definition_location.
There was a problem hiding this comment.
I think this warning should be displayed for any examples defined in spec locations, it'll be more useful.
There was a problem hiding this comment.
I think this warning should be displayed for any examples defined in spec locations, it'll be more useful.
I disagree. I think the improved phrasing being added here would be inaccurate and confusing in cases the locations are not identical. If the locations are not identical, then the user really is defining two groups with the same name, and the original warning (which prints the two different locations and says the new one will overwrite the original one) is accurate and clear.
Consider also that putting a shared example group in a file ending in _spec.rb isn't an error or even something to warn about in general. Maybe the user is defining a scoped shared group within an example group, and accidentally copied and pasted it so that the same group was defined twice in the same spec file. Suggesting they rename their file would be wrong in that case. Or maybe they want to name their support files so that RSpec loads them automatically and they don't have to require them manually. I wouldn't follow that approach myself but there's nothing wrong with it per-se.
|
The build is failing on 1.8.7. I'm not 100% sure but I think it's because the |
devonestes
force-pushed
the
adding-error-message
branch
2 times, most recently
from
37cf321 to
4010f91
Compare
I found myself with a strange warning message not long ago saying the following: ``` WARNING: Shared example group 'a flagger_or_tagger' has been previously defined at: /Users/devoncestes/esh/IRT/spec/services/shared_examples/flagger_or_tagger_spec.rb:10 ...and you are now defining it at: /Users/devoncestes/esh/IRT/spec/services/shared_examples/flagger_or_tagger_spec.rb:10 The new definition will overwrite the original one. ``` Turns out that this was happening because the file in which we defined our shared example group ended in `_spec.rb`, which triggered the Rspec auto loading, thus creating the strange warning. I had no idea how ths could happen until I opened up an issue and @myronmarston pointed out the error. I figured that a helpful warning message that could guide the user to fixing this warning themselves might be good to add, so I've done so here!
devonestes
force-pushed
the
adding-error-message
branch
from
4010f91 to
3657b39
Compare
|
I found the issue with 1.8.7 - I took care of that and the build seems to pass now. I also incorporated your other feedback - thanks for that! I think that warning message is much clearer now. |
|
Thanks @devonestes |
|
I am seeing the old error message (not the helpful one) with rspec-core 3.7.1 for the same reason as you did in issue #2277 Is it possible that this helpful error message is no longer working? |
|
You will see this message only if your shared example is defined inside your spec location, (the check is I'll happily investigate further if you can provide some information to reproduce this |
Add helpful warning message for shared example groups
I found myself with a strange warning message not long ago saying the following:
Turns out that this was happening because the file in which we defined our
shared example group ended in
_spec.rb, which triggered the Rspec autoloading, thus creating the strange warning. I had no idea how this could happen
until I opened up an issue (#2277) and @myronmarston pointed out the error.
I figured that a helpful warning message that could guide the user to fixing
this warning themselves might be good to add, so I've done so here!