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

ungit: remove process.kill/wait #55285

Merged
merged 1 commit into from
May 26, 2020

Conversation

samford
Copy link
Member

@samford samford commented May 25, 2020

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

This removes Process.kill/Process.wait from the test (since brew does its own killing these days), in hopes of avoiding the execution expired timeout (as seen in #55191).

@samford
Copy link
Member Author

samford commented May 25, 2020

Unfortunately, we still need to resolve the AssertionFailedError.

==> FAILED
Testing ungit
/usr/bin/sandbox-exec -f /private/tmp/homebrew20200525-82270-w0879j.sb ruby -W0 -I $LOAD_PATH -- /usr/local/Homebrew/Library/Homebrew/test.rb /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/ungit.rb --verbose
==> curl -s 127.0.0.1:56668/

Killing child processes...
Error: ungit: failed
An exception occurred within a child process:
  Test::Unit::AssertionFailedError: <0> expected but was
<7>.

This test works fine locally but I can't yet see what the problem is on CI to be able to fix it (since print statements don't show up in the log).

@samford samford force-pushed the ungit-remove-process-kill-wait branch from 85a13a1 to 201936d Compare May 26, 2020 04:10
@SMillerDev
Copy link
Member

SMillerDev commented May 26, 2020

Try ohai instead and use ,7) in shell_output.

@samford samford force-pushed the ungit-remove-process-kill-wait branch from 201936d to 90b9b84 Compare May 26, 2020 05:22
@SMillerDev
Copy link
Member

I just looked up what 7 means for curl (CURLE_COULDNT_CONNECT (7)). It could very well be that it just needs to sleep a little longer to properly wait for the server.

@samford samford force-pushed the ungit-remove-process-kill-wait branch from 90b9b84 to 8c657fa Compare May 26, 2020 05:34
Formula/ungit.rb Outdated Show resolved Hide resolved
@samford samford force-pushed the ungit-remove-process-kill-wait branch from 8c657fa to 1410ce7 Compare May 26, 2020 05:47
@samford
Copy link
Member Author

samford commented May 26, 2020

use ,7) in shell_output.

Oh yeah, I forgot that you have to resolve the assertion issue for print statements to actually print. p worked fine when I added , 7) in shell_output.

I just looked up what 7 means for curl (CURLE_COULDNT_CONNECT (7)). It could very well be that it just needs to sleep a little longer to properly wait for the server.

The output was just an empty string ("") when the test failed, so the error makes sense. I tried this with sleep 10 and it still failed in the same way.

Removing --autoShutdownTimeout=6000 seems to resolve the issue, even when using the existing 5 second sleep. I'm going to give this another run, to make sure it's not a fluke.

@samford
Copy link
Member Author

samford commented May 26, 2020

It seems good, so I'm going to go ahead and merge. Thanks for your help, Sean.

@samford samford merged commit 1b70f87 into Homebrew:master May 26, 2020
@samford samford deleted the ungit-remove-process-kill-wait branch May 26, 2020 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants