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

Run the test suite in the default API mode #16895

Open
1 task done
apainintheneck opened this issue Mar 15, 2024 · 11 comments
Open
1 task done

Run the test suite in the default API mode #16895

apainintheneck opened this issue Mar 15, 2024 · 11 comments
Labels
features New features help wanted We want help addressing this

Comments

@apainintheneck
Copy link
Contributor

Verification

Provide a detailed description of the proposed feature

We should run the test suite with the API mode enabled by default and then explicitly specify that we want to exit API mode when needed for individual tests. This essentially flips the way we do things right now where no API is the default and you have to enable the API mode for individual tests.

The escape hatch for non-API friendly tests would be the following.

it "tests something", :without_api do
  # logic
end

What is the motivation for the feature?

We currently set HOMEBREW_NO_INSTALL_FROM_API before running the test suite. This is to avoid any errors that we get from tests that were originally designed to work without the API. The problem is that the API has been the default way to use Homebrew for around a year (Homebrew v4 was launched in February 2023). Ideally our tests would use the default user configuration unless the code being tested can't run in API mode.

How will the feature be relevant to at least 90% of Homebrew users?

We don't have a lot of stats around this but I would assume that 90% of users use Homebrew in API mode. Additional testing with the default configuration settings would make Homebrew more reliable since the tests would more closely mirror how the software actually gets used.

What alternatives to the feature have been considered?

Keeping the current default or specifying with and without API explicitly for each test block.

@apainintheneck apainintheneck added the features New features label Mar 15, 2024
@apainintheneck
Copy link
Contributor Author

Previous discussion can be found here: #16863 (comment)

@apainintheneck apainintheneck added the help wanted We want help addressing this label Mar 15, 2024
@MikeMcQuaid
Copy link
Member

Thanks for opening @apainintheneck!

@reitermarkus
Copy link
Member

Note that we also need to mock the API, otherwise the tests will all actually call out to the API.

Ideally we need some test helper that tests both, after setting up either the tap locally or stubbing the API with the given formulae/casks.

@apainintheneck
Copy link
Contributor Author

There are probably a whole bunch of tests which would run just as happily with or without the API and those should not need to be modified.

Mocking the API would be nice to better represent real world usage but there isn't any easy or generic way to do that since we use a Curl wrapper internally to make HTTP requests. Things like VCR or Webmock won't work for our use case. That said Utils::Curl is not that huge so we could probably share the logic a bit.

IMO that could be handled as a follow-up though and we could just mark those tests without API for the time being and then remove that later on on a test-by-test basis when we're ready.

It also reminds me of the point I brought up in the thread earlier about having a way to validate that no network requests are being made outside of specs tagged :needs_network. If we had something like that, it would make it a lot easier to determine which specs need to be updated or changed beyond obvious failures. I don't have any good ideas about how to implement that though.

@Bo98
Copy link
Member

Bo98 commented Mar 16, 2024

"Works with API" here really only means "it doesn't require a local Homebrew/core clone" and can use the local JSON instead - it doesn't mean we necessarily should be hitting code that sends network requests to the API constantly as that's largely an auto-update thing. HOMEBREW_NO_AUTO_UPDATE is already set during brew tests so the only extra case to worry about in terms of downloads is that if HOMEBREW_CACHE_API is changed to point to a test directory then it may also need a (formula|cask).json fixture (even if that's just []) to avoid a missing-JSON force download. The file in the real location can be verified to exist prior to the tests beginning, if it isn't already.

@reitermarkus
Copy link
Member

Yes, then I mocking the API means mocking that local JSON file.

@MikeMcQuaid
Copy link
Member

HOMEBREW_NO_AUTO_UPDATE is already set during brew tests so the only extra case to worry about in terms of downloads is that if HOMEBREW_CACHE_API is changed to point to a test directory then it may also need a (formula|cask).json fixture (even if that's just []) to avoid a missing-JSON force download.

This sounds ideal 👍🏻

@apainintheneck
Copy link
Contributor Author

Now that #16903 has been merged in we can be a bit more confident that making the API mode the default won't cause problems. Of course, a bunch of tests will still need to be updated regardless. I also added the :no_api scope which sets HOMEBREW_NO_INSTALL_FROM_API for that test block. That should be helpful here too.

@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 23, 2024

"Works with API" here really only means "it doesn't require a local Homebrew/core clone" and can use the local JSON instead - it doesn't mean we necessarily should be hitting code that sends network requests to the API constantly as that's largely an auto-update thing. HOMEBREW_NO_AUTO_UPDATE is already set during brew tests so the only extra case to worry about in terms of downloads is that if HOMEBREW_CACHE_API is changed to point to a test directory then it may also need a (formula|cask).json fixture (even if that's just []) to avoid a missing-JSON force download. The file in the real location can be verified to exist prior to the tests beginning, if it isn't already.

I think it's a little trickier than that. We also have to sign the blank JSON fixtures as well and recreate them before each test.

Edit: We can just sign them once and then copy them over before each test. Or we could have them persist between tests.

@apainintheneck
Copy link
Contributor Author

We could potentially just have a pair of public and private keys that are only used for signing test files. Not sure how much better that is than a little mocking though.

@MikeMcQuaid
Copy link
Member

We could potentially just have a pair of public and private keys that are only used for signing test files. Not sure how much better that is than a little mocking though.

Yeh, that would work.

If mocking gets this over the line quicker: I'm definitely less opposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features New features help wanted We want help addressing this
Projects
None yet
Development

No branches or pull requests

4 participants