Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

[tests] Add missing calls to super in setup and teardown methods #949

Merged
merged 1 commit into from
Oct 30, 2020

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Oct 30, 2020

This can lead to intermittent failures depending on the order tests are run in.

@andyw8 andyw8 requested review from a team as code owners October 30, 2020 19:15
@andyw8 andyw8 changed the title [tests] Add missing calls to super in setup methods [tests] Add missing calls to super in setup and teardown methods Oct 30, 2020
project_context('project')
::ShopifyCli::Project.clear
super
end

def teardown
unless @minitest_ext_setup_called
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this to find the missing places.

@@ -14,6 +14,7 @@ def setup
def teardown
@context.setenv('GEM_HOME', nil)
@context.setenv('GEM_PATH', nil)
super
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For missing super calls in teardown, I don't know of an automated way of finding these, I just manually checked them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. If minitest creates a new test instance on every run there won't be any state left over on subsequent runs, so the only place I can think of would be to check the flag during garbage collection (if that's even possible).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe use an at_exit hook.

But I'd like for this to be a stopgap measure, and use a linter instead.

I've added a entry to the minitest style guide:
rubocop/minitest-style-guide#35

So the next step would be to add it in rubocop-minitest:
rubocop/rubocop-minitest#111

@andyw8 andyw8 force-pushed the andyw8/add-missing-setup-super-calls branch from c8947c8 to 468874b Compare October 30, 2020 19:24
@@ -14,6 +14,7 @@ def setup
def teardown
@context.setenv('GEM_HOME', nil)
@context.setenv('GEM_PATH', nil)
super
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. If minitest creates a new test instance on every run there won't be any state left over on subsequent runs, so the only place I can think of would be to check the flag during garbage collection (if that's even possible).

@andyw8 andyw8 merged commit 1a5b2d7 into master Oct 30, 2020
@andyw8 andyw8 deleted the andyw8/add-missing-setup-super-calls branch October 30, 2020 20:09
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.

2 participants