Update tests failing on master #4575
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
|
Changes look good! What part of the PR wasn't updated? Do you mean you had local commits ready and didn't push up to your branch on GitHub yet? |
agjohnson
approved these changes
Aug 28, 2018
|
The PR was done before #4456, so it was using the old interface. Then the new interface was merged, and the tests were keeping the old interface. |
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.
Ref to #4461 (comment)
So, this was caused because the PR was outdated, makes me think if we could do better to prevent this. What about requiring the PRs to be updated before merging to master? Now GitHub supports a button to update the PR from the web, so the process isn't slow, but we will need to wait to the tests to pass again with the new merge, for simple PRs this can be a little overkill, but in general, I think this is a sane check