feat: add rubocop to enforce webmocks are enabled only after test_helper

[stephenliang]

When using webmock to mock out web requests, webmock will error if any live web request is made without being stubbed. test_helper does make web requests and will therefore cause webmock to fail. If test_helper is initialized by another a test, the webmock will not fail.

To ensure consistency, ensure that test_helper is required prior to webmock.

Links

• slack thread

Testing story

Ran rubocop and autocorrect, spot inspection.

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

• Tests provide adequate coverage
• Privacy and Security impacts have been assessed
• Code is well-commented
• New features are translatable or updates will not break translations
• Relevant documentation has been added or updated
• User impact is well-understood and desirable
• Pull Request is labeled appropriately
• Follow-up work items (including potential tech debt) are tracked and linked

[cat5inthecradle]

relaying from discussion in the infra checkin - is there a way to simply enforce that all implementation code happens after imports?

[stephenliang]

relaying from discussion in the infra checkin - is there a way to simply enforce that all implementation code happens after imports?

By implementation code, are you referring to the code in test_helper? Perhaps there are some opportunities to refactor how test_helper works.

[cat5inthecradle]

Nah, just speaking in super general terms. Similar to how we have added import order rules to our frontend code, is there a linting solution here that would require require statements to always come before implementation code like Webmock.disable_net_connect

[stephenliang]

Nah, just speaking in super general terms. Similar to how we have added import order rules to our frontend code, is there a linting solution here that would require require statements to always come before implementation code like Webmock.disable_net_connect

I think that makes sense for the most part. However, there are some uncommon require usage as part of application code such as conditional requires. I don't think the Frontend code is enforcing that imports come before application code, but rather are sectioned and ordered according to certain rules. Occassionally the frontend code will want to asynchronously load imports in the application (probably more common than in RoR).

Perhaps we can discuss this more at developer tools working group? For now, we can clean up and prevent this test build failure from happening again on these 7 tests.

Existing usages for require in the application code:

1. ./app/controllers/admin_reports_controller.rb
2. ./app/models/unit.rb
3. ./config/initializers/devise.rb
4. ./config/application.rb
5. ./lib/tasks/seed.rake

[cat5inthecradle]

LGTM but would love input from someone in the code more often than me.

[davidsbailey]

I think this approach is probably good enough. IIRC we run lint in the following scenarios:

• lint modified files during developer commit hooks and in drone
• lint all files during the staging build

in the future, if we start including webmock so often that we want to move it into test_helper.rb, then it seems like we could instead do something like this in test_helper.rb:

• check that webmock has not been required
• do whatever needs to be done before webmock is required
• require webmock

[davidsbailey]

a 3rd option would be to write our own cdo/webmock.rb wrapper which does all the necessary setup and requires webmock, and then we can add a lint rule which disallows requiring webmock directly from outside of this wrapper.

[davidsbailey]

just clarifying I think this option looks good as is :-) the other options are to keep in mind for the future