Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Run more assertions in more cases #6707

Merged
2 commits merged intomasterfrom
more_assertions
Sep 25, 2018
Merged

Run more assertions in more cases #6707
2 commits merged intomasterfrom
more_assertions

Conversation

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

What was the end-user problem that led to this PR?

I noticed a couple of places where assertions were being excluded and they shouldn't:

  • One was introduced by me in Better --force to --redownload transition #6702, where the specs added (and some already present) started being tested only on bundler 2.x.
  • The other one was introduced in f7414bc, where one assertion would be run only if a certain env variable was not set. I think it was because of a TravisCI environmental issue that now seems fixed.

What was your diagnosis of the problem?

My diagnosis was that none of these exclusions are necessary.

What is your fix for the problem, implemented in this PR?

My fix is to restore the excluded assertions to all environments and branches.

Why did you choose this fix out of the possible options?

I chose this fix because it seems best to avoid future problems.

I think this was a bad fix and it's no longer necessary.
So they are run on bundler 1.x too.
@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

I noticed the issue with #6702 when I was going to milestone it for 1.17.0. The changes here will only actually be exercised when this PR and #6702 are backported. I already did that locally and the specs pass.

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

@bundlerbot r+

ghost pushed a commit that referenced this pull request Sep 25, 2018
6707: Run more assertions in more cases  r=deivid-rodriguez a=deivid-rodriguez

### What was the end-user problem that led to this PR?

I noticed a couple of places where assertions were being excluded and they shouldn't:

* One was introduced by me in #6702, where the specs added (and some already present) started being tested only on bundler 2.x.
* The other one was introduced in f7414bc, where one assertion would be run only if a certain env variable was not set. I think it was because of a TravisCI environmental issue that now seems fixed.

### What was your diagnosis of the problem?

My diagnosis was that none of these exclusions are necessary.

### What is your fix for the problem, implemented in this PR?

My fix is to restore the excluded assertions to all environments and branches.

### Why did you choose this fix out of the possible options?

I chose this fix because it seems best to avoid future problems.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
@ghost
Copy link
Copy Markdown

ghost commented Sep 25, 2018

Build succeeded

@ghost ghost merged commit 371ff51 into master Sep 25, 2018
@deivid-rodriguez deivid-rodriguez deleted the more_assertions branch September 25, 2018 11:48
@deivid-rodriguez deivid-rodriguez added this to the 1.17.0 milestone Oct 6, 2018
@colby-swandale
Copy link
Copy Markdown
Member

why is this in the 1.17.0 milestone?

@deivid-rodriguez
Copy link
Copy Markdown
Contributor Author

Because otherwise the specs I'm adding here for bundler < 2 will never be run, and I'd like to make sure I'm not adding any deprecation messages here (yet).

@colby-swandale
Copy link
Copy Markdown
Member

colby-swandale commented Oct 9, 2018

thanks for the explanation

colby-swandale pushed a commit that referenced this pull request Oct 9, 2018
6707: Run more assertions in more cases  r=deivid-rodriguez a=deivid-rodriguez

I noticed a couple of places where assertions were being excluded and they shouldn't:

* One was introduced by me in #6702, where the specs added (and some already present) started being tested only on bundler 2.x.
* The other one was introduced in f7414bc, where one assertion would be run only if a certain env variable was not set. I think it was because of a TravisCI environmental issue that now seems fixed.

My diagnosis was that none of these exclusions are necessary.

My fix is to restore the excluded assertions to all environments and branches.

I chose this fix because it seems best to avoid future problems.

Co-authored-by: David Rodríguez <deivid.rodriguez@riseup.net>
(cherry picked from commit 3d9e616)
colby-swandale pushed a commit that referenced this pull request Oct 25, 2018
* 1-17-stable: (38 commits)
  Version 1.17.0 with changelog
  Merge #6754
  Version 1.17.0.pre.2 with changelog
  fix failing bundle remove specs
  Still document the `--force` option
  scope specs testing bundler 2 deprecations to bundler 1 only
  Merge #6718
  Merge #6707
  Merge #6702
  Merge #6316
  Auto merge of #6447 - agrim123:agr-update-error-message, r=segiddins
  Auto merge of #6513 - agrim123:agr-bundler-remove, r=indirect
  Auto merge of #6318 - jhawthorn:fix_incorrect_test_in_requires_sudo, r=segiddins
  Auto merge of #6450 - bundler:segiddins/bundle-binstubs-all, r=colby-swandale
  Auto merge of #6024 - gwerbin:custom-user-bundle-dir, r=colby-swandale
  Version 1.17.0.pre.1 with changelog
  Auto merge of #5964 - bundler:colby/deprecate-viz-command, r=segiddins
  Auto merge of #5986 - bundler:seg-jobs-count, r=indirect
  Auto merge of #5995 - bundler:seg-gvp-major, r=indirect
  Auto merge of #5803 - igorbozato:path_relative_to_pwd, r=indirect
  ...
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants