Move #738 regression check to spec #1565
Merged
Conversation
This file contains hidden or 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 is confusing and not useful as documentation, all #738 was intended to do was to prevent our let definitions from being defined earlier than 3rd party inclusions (and hence being overriden) which was confusing behaviour, so move it to a spec.
|
Did you confirm that this spec fails if you revert the fix from #738? Not absolutely necessary, but it's always nice to check that this spec will catch a regression here. I've seen enough subtle differences between the spec environment and the cucumber environment that it's hard to tell w/o actually trying it. |
|
I didn't, but I did check how the behaviour was affected by including in various places vs the let definition and I'm confident this spec represents the behaviour we expect. We could remove this check altogether if you like because the fix was actually reverted after an update to rspec-core... |
|
LGTM except for the travis failure. |
JonRowe
added a commit
that referenced
this pull request
Feb 28, 2016
Move #738 regression check to spec
|
Merging despite build failure, as that happens to be unrelated (master also fails) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is confusing and not useful as documentation, all #738 was intended
to do was to prevent our let definitions from being defined earlier than
3rd party inclusions (and hence being overridden) which was confusing
behaviour, so move it to a spec.
/cc @samphippen @myronmarston