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

cash-cli: Fix test #75709

Merged
merged 2 commits into from Apr 23, 2021
Merged

cash-cli: Fix test #75709

merged 2 commits into from Apr 23, 2021

Conversation

n-thumann
Copy link
Contributor

@n-thumann n-thumann commented Apr 21, 2021

Fixes failing test in e.g. #75705.
The error is caused by the default API that cash-cli uses and now always requires an API key. Unfortunately, there's no other way for testing than using a dumb version check, but (according to the Formula Cookbook) this is better than nothing and definitely better than a failing test.


  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@BrewTestBot BrewTestBot added the nodejs Node or npm use is a significant feature of the PR or issue label Apr 21, 2021
@carlocab carlocab mentioned this pull request Apr 21, 2021
@carlocab
Copy link
Member

So the error is expected then? If so, we can keep the existing test, and just modify it to account for exit code and output from failure.

@carlocab carlocab changed the title cash-cli: Fixes test cash-cli: Fix test Apr 21, 2021
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks for this, @n-thumann. We can do better than a version test here, now that we know that the failure is expected.

Formula/cash-cli.rb Outdated Show resolved Hide resolved
@n-thumann
Copy link
Contributor Author

I wouldn't say that the error is expected. It seems like 20 days ago exchangeratesapi.io suddenly starts requiring an API key and the author of cash-cli hasn't handles that yet. I'm not sure if this will be fixed soon as the last commit on master in 17 months old :/

@carlocab carlocab mentioned this pull request Apr 21, 2021
5 tasks
@carlocab
Copy link
Member

carlocab commented Apr 21, 2021

I wouldn't say that the error is expected. It seems like 20 days ago exchangeratesapi.io suddenly starts requiring an API key and the author of cash-cli hasn't handles that yet. I'm not sure if this will be fixed soon as the last commit on master in 17 months old :/

I see. In that case, we definitely shouldn't change the test. Otherwise we have no way of knowing whether this problem ever gets fixed or not.

@n-thumann
Copy link
Contributor Author

n-thumann commented Apr 21, 2021

Looks like there are some issues matching against the output

==> /usr/local/Cellar/cash-cli/4.2.1/bin/cash 100 USD PLN CHF
- Converting...
✖ Something went wrong :(

Error: cash-cli: failed
An exception occurred within a child process:
  Minitest::Assertion: Expected /Something\ went\ wrong\ :\(/ to match "\n".

Formula/cash-cli.rb Outdated Show resolved Hide resolved
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@carlocab carlocab mentioned this pull request Apr 22, 2021
5 tasks
@n-thumann n-thumann requested a review from carlocab April 22, 2021 21:13
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

My opinion hasn't really changed from what I said above: #75709 (comment)

That the test fails when you try it seems to be the correct result, as it appears that this software is broken right now.

@carlocab
Copy link
Member

@SMillerDev thoughts here? Do we want to change the test?

@SMillerDev
Copy link
Member

I think it should be deprecated as unmaintained and the test adjusted with a comment that they are no longer fully accurate.

@carlocab
Copy link
Member

Works for me. @n-thumann, do you mind making those changes here?

@n-thumann
Copy link
Contributor Author

Sure, give me a sec :)

@BrewTestBot BrewTestBot added the formula deprecated Formula deprecated label Apr 23, 2021
@carlocab carlocab added the CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. label Apr 23, 2021
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks, @n-thumann.

@carlocab carlocab merged commit bf84bd6 into Homebrew:master Apr 23, 2021
@carlocab carlocab mentioned this pull request Apr 23, 2021
@github-actions github-actions bot added the outdated PR was locked due to age label May 24, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2021
@n-thumann n-thumann deleted the fix_cash-cli_test branch September 4, 2021 16:16
@n-thumann n-thumann restored the fix_cash-cli_test branch September 4, 2021 16:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-syntax-only Change only affects brew syntax, not the install. Only run syntax CI. formula deprecated Formula deprecated nodejs Node or npm use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants