Skip to content

cash-cli: Fix test#75709

Merged
carlocab merged 2 commits intoHomebrew:masterfrom
n-thumann:fix_cash-cli_test
Apr 23, 2021
Merged

cash-cli: Fix test#75709
carlocab merged 2 commits intoHomebrew:masterfrom
n-thumann:fix_cash-cli_test

Conversation

@n-thumann
Copy link
Copy Markdown
Contributor

@n-thumann n-thumann commented Apr 21, 2021 •
edited
Loading

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
Closed
@carlocab
Copy link
Copy Markdown
Member

carlocab commented Apr 21, 2021

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
carlocab
carlocab requested changes Apr 21, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@n-thumann
Copy link
Copy Markdown
Contributor Author

n-thumann 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 :/

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

carlocab commented Apr 21, 2021 •
edited
Loading

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
Copy Markdown
Contributor Author

n-thumann commented Apr 21, 2021 •
edited
Loading

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".

carlocab
carlocab reviewed Apr 21, 2021
View reviewed changes
@n-thumann @carlocab
cash-cli: Fix test
d5c8019 
Co-authored-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@carlocab carlocab mentioned this pull request Apr 22, 2021
Closed
5 tasks
@n-thumann n-thumann requested a review from carlocab April 22, 2021 21:13
carlocab
carlocab reviewed Apr 23, 2021
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

carlocab commented Apr 23, 2021

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

@SMillerDev
Copy link
Copy Markdown
Member

SMillerDev commented Apr 23, 2021

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
Copy Markdown
Member

carlocab commented Apr 23, 2021

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

@n-thumann
Copy link
Copy Markdown
Contributor Author

n-thumann commented Apr 23, 2021

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
carlocab
carlocab approved these changes Apr 23, 2021
View reviewed changes
Copy link
Copy Markdown
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
Closed
@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.

Reviewers

@carlocab carlocab carlocab approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@n-thumann @carlocab @SMillerDev @BrewTestBot