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.

Sign up for GitHub

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

Jump to bottom

Prevent unexpected network calls in tests #16903

Merged
merged 3 commits into from
Mar 21, 2024
Merged

Prevent unexpected network calls in tests #16903

merged 3 commits into from
Mar 21, 2024

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Mar 16, 2024 •
edited
Loading

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This implemented a crude check for unexpected network calls in tests that are not tagged :needs_network. Basically, it raise an error if any method the .curl_executable method in the Utils::Curl module is referenced and is not stubbed. This works most of the time and it did help me uncover a few tests that were making network calls when they shouldn't have been. Of course, sometimes we know that the network calls we're going to make are all to the local filesystem and in that case there is an escape hatch you can use with the :allow_utils_curl :needs_utils_curl tag.

As written this works well with the current codebase but does clash a bit with the API mocking ideas found in #16895. It should be possible to override the checks in the test suite when we add some sort of mock API helper though.

I'm marking this as discussion to see if people think this is a good general idea and are okay with the implementation.

CC: @Bo98 @reitermarkus

Changes:

  • b66097f
    • Add check for unexpected network calls to spec_helper.rb
    • Fix acceptable tests that use curl to download local files
  • ad35db4
    • Fix tests with unexpected network calls
    • IMO these should be merged in whether or not we decide to include unexpected network call check

@apainintheneck apainintheneck added the discussion Input solicited from others label Mar 16, 2024
MikeMcQuaid
MikeMcQuaid approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Excuse the nit-picking: LOVE this approach, really clever idea and will be super useful!

Library/Homebrew/test/dev-cmd/cat_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/formulary_spec.rb
Comment on lines +388 to +389
allow(Homebrew::API::Formula).to receive_messages(all_aliases: {}, all_renames: {})
allow(CoreTap.instance).to receive(:tap_migrations).and_return({})
Copy link
Member

Choose a reason for hiding this comment

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

Might be nicer to ensure some correct files are written on disk instead of mocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Out of laziness, I'm going to skip that for now.

Library/Homebrew/test/dev-cmd/edit_spec.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/spec_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/spec_helper.rb Outdated Show resolved Hide resolved
Library/Homebrew/test/spec_helper.rb Outdated Show resolved Hide resolved
@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 19, 2024 •
edited
Loading

I think this check can actually be much much simpler. All of the network calls in Utils::Curl require you to first get the curl executable path since we use either brewed curl or shimmed curl. That means we can probably just check Utils::Curl.curl_executable to prevent any network calls in this module specifically.

There are other places in the codebase where network calls could potentially happen in tests like the DownloadStrategy module. I'll see if there's some easy way to check in that module too.

Edit: It's not simple to check in DownloadStrategy so punting on that for now.

@apainintheneck apainintheneck force-pushed the prevent-unexpected-network-calls-in-tests branch from 0ad15b6 to 1adddc5 Compare March 19, 2024 03:55
@apainintheneck
spec_helper: add check for unexpected network calls
b66097f 
Any test that is not tagged as :needs_network and that makes
a call to an unapproved method in the `Utils::Curl` module
will raise an error unless that method gets mocked somehow.

tests: add exceptions for tests that use curl to download local files

These are acceptable ways to use curl in local, non-network tests.
For those edge cases we allow you to bypass the check with :needs_utils_curl.
@apainintheneck
tests: fix tests that make unexpected network calls
ad35db4 
These were found with the Utils::Curl check and just turning
off the network on my computer and running the entire test suite.
@apainintheneck apainintheneck force-pushed the prevent-unexpected-network-calls-in-tests branch from 1adddc5 to ad35db4 Compare March 19, 2024 04:00
@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 19, 2024 •
edited
Loading

@MikeMcQuaid Thanks for the feedback! I was able to simplify the check further as explained above along with using your suggestions for the error message and the name of the escape hatch which is now :needs_utils_curl.

I also removed the changes to the two tests which didn't need the network anymore based on the bug fix in Homebrew::CLI::NamedArgs.

@MikeMcQuaid
Copy link
Member

That means we can probably just check Utils::Curl.curl_executable to prevent any network calls in this module specifically.

Or make it so that it always returns e.g. /usr/bin/false.

@apainintheneck apainintheneck marked this pull request as ready for review March 20, 2024 01:32
@apainintheneck
spec_helper: add :no_api test scope
74aea8e 
This sets the HOMEBREW_NO_INSTALL_FROM_API environment variable
to prevent the selected tests from using the API. We will need
this as we transition to having the API be enabled by default
when running the tests but it's also nice as a sanity check
with the :needs_utils_curl scope in a few places.
MikeMcQuaid
MikeMcQuaid reviewed Mar 20, 2024
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great once s/:needs_github_api, //g is done, sorry/thanks!

@apainintheneck apainintheneck force-pushed the prevent-unexpected-network-calls-in-tests branch from 836a26b to 74aea8e Compare March 21, 2024 03:29
@apainintheneck apainintheneck merged commit 7b2bfee into Homebrew:master Mar 21, 2024
25 checks passed
@apainintheneck apainintheneck mentioned this pull request Mar 21, 2024
Open
1 task
@MikeMcQuaid
Copy link
Member

Thanks @apainintheneck!

@github-actions github-actions bot added the outdated PR was locked due to age label Apr 21, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid approved these changes

@Bo98 Bo98 Bo98 approved these changes

@reitermarkus reitermarkus Awaiting requested review from reitermarkus

Assignees
No one assigned
Labels
discussion Input solicited from others outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apainintheneck @MikeMcQuaid @Bo98 @reitermarkus