Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify after blocks in specs #3702

Merged
merged 4 commits into from
Sep 23, 2019
Merged

Simplify after blocks in specs #3702

merged 4 commits into from
Sep 23, 2019

Conversation

javierm
Copy link
Member

@javierm javierm commented Sep 13, 2019

Objectives

  • Remove unnecessary after blocks cleaning up the database
  • Simplify before and after blocks in tests using delayed jobs

@javierm javierm added the Specs label Sep 13, 2019
@javierm javierm self-assigned this Sep 13, 2019
@javierm javierm added this to Reviewing in Roadmap via automation Sep 13, 2019
@javierm javierm requested a review from taitus September 13, 2019 23:13
@javierm javierm force-pushed the remove_after_blocks branch 2 times, most recently from 64db5fa to ce2c9eb Compare September 23, 2019 11:13
Settings are stored in the database, and so any changes to the settings
done during the tests are automatically rolled back between one test and
the next one.

There were also a few places where we weren't using an `after` block but
changing the setting at the end of the test.
We already configure `I18n.locale` and we reset Globalize's fallbacks
before every test.

On the other hand, RSpec automatically resets anything which is stub
with `allow`, so there's no need to use `and_call_original` in an
`after` block.
Among other advantages, now we can run these tests with
`rspec --tag delayed_jobs`.
These settings are enabled by default.

It could be argued explicitely enabling the features makes tests more
consistent, because they'll work if we change the default setting. It
could also be argued that it makes tests more expressive because it
makes the reader realize certain things will only work if a setting is
enabled.

However, we were only doing so in a few tests. The truth is, thousands
of our tests depend on certain features being enabled. So IMHO we should
be consistent and either set them on every test, or not at all. I'm
choosing the latter option for simplicity.
@javierm javierm merged commit 1faa659 into master Sep 23, 2019
Roadmap automation moved this from Reviewing to Release 1.1.0 Sep 23, 2019
@javierm javierm deleted the remove_after_blocks branch September 23, 2019 14:52
smarques pushed a commit to venetochevogliamo/consul that referenced this pull request Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Roadmap
  
Release 1.1.0
Development

Successfully merging this pull request may close these issues.

None yet

2 participants