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

test_integration_cmds: allow testing offline. #430

Merged
merged 1 commit into from
Jul 4, 2016
Merged

test_integration_cmds: allow testing offline. #430

merged 1 commit into from
Jul 4, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jun 30, 2016

  • 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 tests with your changes locally?

Set HOMEBREW_NO_GITHUB_API to allow running all integration command tests (search specifically for now) offline.

CC @eirinikos

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jun 30, 2016
@sjackman
Copy link
Contributor

👍 Thanks, Mike.

@@ -46,6 +46,7 @@ def cmd_output(*args)
ENV["HOMEBREW_BREW_FILE"] = HOMEBREW_PREFIX/"bin/brew"
ENV["HOMEBREW_INTEGRATION_TEST"] = cmd_id_from_args(args)
ENV["HOMEBREW_TEST_TMPDIR"] = TEST_TMPDIR
ENV["HOMEBREW_NO_GITHUB_API"] = "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to cmd/tests.rb and hide it behind an --offline argument? I'm not too happy disabling this unconditionally and only for the integration tests (some other parts of the tests might exercise the GitHub API in the future) as I consider this to be a temporary measure until we have a better solution for offline testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think no tests shouldn't require you to be online to pass but it's currently only integration tests that have this issue. I could instead add this to brew tests and apply it globally but I'd rather the flag be --online rather than --offline for that reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed; this makes more sense. I mainly wanted to avoid special casing the integration tests (even though they are currently the only ones that do require a network connection).

👍 on setting HOMEBREW_NO_GITHUB_API in cmd/tests.rb unless --online is passed (also better aligns with brew audit where we also use --online).

@MikeMcQuaid
Copy link
Member Author

👍 on setting HOMEBREW_NO_GITHUB_API in cmd/tests.rb unless --online is passed (also better aligns with brew audit where we also use --online).

Did this.

@UniqMartin
Copy link
Contributor

Thanks! Looks good to me! 👍

Would you mind sneaking in this additional option into the help text at the top of the file (maybe in post)?

Set HOMEBREW_NO_GITHUB_API to allow running all tests (but search's
integration test specifically for now) offline. This can be overridden
with `--online`.
@MikeMcQuaid MikeMcQuaid merged commit 3e982b9 into Homebrew:master Jul 4, 2016
@MikeMcQuaid MikeMcQuaid deleted the integration-cmds-test-offline branch July 4, 2016 12:32
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 4, 2016
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
Set HOMEBREW_NO_GITHUB_API to allow running all tests (but search's
integration test specifically for now) offline. This can be overridden
with `--online`.
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
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.

4 participants