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

fanyi: fix test for Linux #113248

Closed
wants to merge 1 commit into from

Conversation

n-thumann
Copy link
Contributor

@n-thumann n-thumann commented Oct 16, 2022

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • 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 --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This PR fixes the test of fanyi for Linux.
Part of #86422.

Fancy will pronunciate the word by default. On Linux however, this requires some additional text-to-speech libraries to be installed, hence the test fails. On macOS this works without any additional libraries.
I therefore disabled the acoustic pronunciation, because this isn't useful for testing anyways (and also fixed a typo). This can be also considered unwanted, because a test for this Formula shouldn't cause you Mac to play sounds.

@BrewTestBot BrewTestBot added the nodejs Node or npm use is a significant feature of the PR or issue label Oct 16, 2022
@carlocab carlocab added the CI-no-bottles Merge without publishing bottles label Oct 16, 2022
@carlocab
Copy link
Member

This formula actually already has a Linux bottle, so the test succeeded on Linux in CI at some point.

@n-thumann
Copy link
Contributor Author

Hm, you're right!
I have no idea how the pipeline succeeded in the past, since I am able to reproduce this on both vanilla Debian Bullseye and also with the Homebrew Ubuntu 22.04 Docker image used in the pipeline 🤔
Let me check something...

@n-thumann
Copy link
Contributor Author

The test on Ubuntu still runs through (https://github.com/Homebrew/homebrew-core/actions/runs/3260182703/jobs/5353576582), even without --no-say, despite failing locally for me.
Closing this, since there seems to be no immediate reason for this patch. Feel free so reopen, if this changes (I'll then remove my testing commit for this to be merged).

@n-thumann n-thumann closed this Oct 16, 2022
@Bo98
Copy link
Member

Bo98 commented Oct 16, 2022

If it fixes an issue locally, then I'm fine with merging this.

@n-thumann
Copy link
Contributor Author

n-thumann commented Oct 17, 2022

If it fixes an issue locally, then I'm fine with merging this.

Great! Reopened & remove test commit.
Forgot to mention: It also prevents you Mac from talking to you while running the test on macOS :D

@carlocab
Copy link
Member

It also prevents you Mac from talking to you while running the test on macOS :D

Sounds like a regression

@BrewTestBot
Copy link
Member

🤖 A scheduled task has triggered a merge.

@n-thumann
Copy link
Contributor Author

If you don't mind, could you add hacktoberfest-accepted? ✌🏻

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI-no-bottles Merge without publishing bottles hacktoberfest-accepted 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.

4 participants