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

tty-share 0.6.2 (new formula) #48833

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@elisescu
Copy link

elisescu commented Jan 9, 2020

Signed-off-by: Vasile Popescu elisescu@elisescu.com

  • 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>)?

@SMillerDev

This comment has been minimized.

Copy link
Member

SMillerDev commented Jan 10, 2020

Segfaults in the test step.

@elisescu elisescu force-pushed the elisescu:tty-share branch from 3aad0e6 to 1ad214a Jan 10, 2020
@elisescu

This comment has been minimized.

Copy link
Author

elisescu commented Jan 10, 2020

Segfaults in the test step.

Yeah, I didn't realize that the formula test will run in a CI environment, where the stdin is not a tty, so my formula test was broken when running in Jenkins (tty was nil).

So I added changed it to check for a --version argument. Hope this will work.

Thx :)

Copy link
Member

SMillerDev left a comment

A segfault if no TTY is present seems like an upstream issue (i'd expect something like a warning instead?)

Formula/tty-share.rb Outdated Show resolved Hide resolved
Formula/tty-share.rb Outdated Show resolved Hide resolved
Signed-off-by: Vasile Popescu <elisescu@elisescu.com>
@elisescu elisescu force-pushed the elisescu:tty-share branch from 1ad214a to 5ac9ad0 Jan 11, 2020
@elisescu elisescu changed the title tty-share 0.5 (new formula) tty-share 0.6.2 (new formula) Jan 16, 2020
@elisescu

This comment has been minimized.

Copy link
Author

elisescu commented Jan 16, 2020

@SMillerDev, the issues should be solved now (both with building, and also the upstream crash).
Anything else that keeps it from merging?

Copy link
Member

SMillerDev left a comment

Please update the test

end

test do
assert_match version.to_s, shell_output("#{bin}/tty-share --version")

This comment has been minimized.

Copy link
@SMillerDev

SMillerDev Jan 17, 2020

Member

We need a test that exercises the some of the functionality of the app. Version checks or usage checks (foo --version or foo --help) are not sufficient, as explained in the formula cookbook.

Could you check if it fails to use tty instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.