This repository was archived by the owner on Apr 14, 2021. It is now read-only.
Merged
Conversation
deivid-rodriguez
force-pushed
the
fix_deprecation_specs
branch
3 times, most recently
from
efe73ef to
2760b9c
Compare
Contributor
Author
|
This is ready. I also have some other PRs ready for each of the remaining deprecations, but I want to get this in first so those PRs can really prove what they are fixing by correcting the skipped specs that this PR is leaving. |
Since we haven't even yet deprecated the old behavior.
Not necessary for the test since the main before block already creates one.
The previous logic was broken. Previously, changing the expectation
about gems.rb vs Gemfile deprecation to
```ruby
expect(warnings).to have_major_deprecation("gems.rb is preferred to chocolate ice-cream")
```
would still pass.
And skip the rest of the broken ones.
Those deprecation messages are no longer printed.
The helper already considers the stream errors are printed to.
I'll get to this on a separate PR.
deivid-rodriguez
force-pushed
the
fix_deprecation_specs
branch
from
2760b9c to
2803075
Compare
Contributor
Author
|
I don't think there's anything that can cause issues here, and it unblocks another PRs, so I'm going to go ahead and merge! Please feel free to leave feedback afterwards and I can amend anything later! 👍 |
Contributor
Author
|
@bundlerbot r+ |
ghost
pushed a commit
that referenced
this pull request
Feb 28, 2019
6991: Fix deprecation specs r=deivid-rodriguez a=deivid-rodriguez
### What was the end-user problem that led to this PR?
The problem was the current deprecation specs are broken. Not the deprecations themselves, their specs.
### What was your diagnosis of the problem?
My diagnosis was we need to fix the specs for the deprecations before actually making sure that the deprecations themselves are properly enabled.
### What is your fix for the problem, implemented in this PR?
My fix is to:
* Get the target version corrected. Deprecation specs should run on bundler 2, not on bundler 1, since deprecations are disabled on bundler 1.
* Get the deprecation matcher fixed. The previous version was passing every time any deprecation was printed and a positive expectation was set. That means that on a spec printing a single deprecation, the assertion `expect(warnings).to have_major_deprecation("bananas will be deprecated in favor of potatoes")`, or any other message, would just pass. This explains why the deprecation specs were currently passing on bundler 1 and it's fixed by 33383f2.
### Why did you choose this fix out of the possible options?
This fix changes the target for the deprecation specs to bundler 2 (e54ddb6) and uses the following strategy to get CI green after that:
* If the deprecation is already correct on bundler 2, do nothing, the specs already pass.
* If the deprecation is incorrect on bundler 2, but the fix is a no-brainer, fix it here. For example, the `bundle update --all` deprecation (968fe96), or the `bundle console` deprecation (5bece2f).
* If the deprecation is incorrect on bundler 2, but fixing it requires a separate discussion, skip the spec for now, and stage the fix for a separate PR.
I chose this fix because it keeps this PR simple and focused, while unblocking fixing the rest of the deprecations by at least having correct (although disabled) specs for them.
Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
Build succeeded |
ghost
merged commit
5 tasks
This pull request was closed.
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
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.
What was the end-user problem that led to this PR?
The problem was the current deprecation specs are broken. Not the deprecations themselves, their specs.
What was your diagnosis of the problem?
My diagnosis was we need to fix the specs for the deprecations before actually making sure that the deprecations themselves are properly enabled.
What is your fix for the problem, implemented in this PR?
My fix is to:
expect(warnings).to have_major_deprecation("bananas will be deprecated in favor of potatoes"), or any other message, would just pass. This explains why the deprecation specs were currently passing on bundler 1 and it's fixed by 33383f2.Why did you choose this fix out of the possible options?
This fix changes the target for the deprecation specs to bundler 2 (e54ddb6) and uses the following strategy to get CI green after that:
bundle update --alldeprecation (968fe96), or thebundle consoledeprecation (5bece2f).I chose this fix because it keeps this PR simple and focused, while unblocking fixing the rest of the deprecations by at least having correct (although disabled) specs for them.