-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
mpd: add test & install example conf file #39360
Conversation
sleep 2 | ||
Process.kill("SIGINT", pid) | ||
Process.wait(pid) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this fail? Will we get an exception back if mpd terminates abnormally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should do. If it doesn't launch correctly, say because it can't find the correct linked dylib
it shouldn't start the process, and there shouldn't be anything to kill. I'll revert my icu4c
tonight and then install mpd
again and then upgrade icu4c
and run the test and see if it blows up as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Garg. Test succeeds even when it shouldn't. Will have to try and find another way, if there is one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I don't know. We could go ugly and obvious as in:
test do
icu4c = Formula["icu4c"].opt_lib/"libicui18n.55.dylib"
assert_match /#{icu4c}/, shell_output("otool -L #{bin}/mpd")
end
But everyone involved, me, you, all of the maintainers aside you, etc are going to hate the test. It works, but I don't really want to do it. Going to cc @jacknagel in case he knows of any Ruby magic on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a server or similar? If so, can it be tested by curl
? Also Process.kill/wait
should be inside a ensure
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, kinda.
I tried testing it with curl
but inside Homebrew it vomits permission denied messages at me. If I run the curl
check outside of Homebrew, it works great. Inside the test do
block? Failure, so much failure.
And yup, my bad on forgetting the ensure
😺.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out I'm an idiot. I forgot to tuck the curl
check inside a begin
block and that was causing the failure. This should actually fix Tim's concern earlier as well, because curl
will fail if the server doesn't respond and that'll blow up the test, so we should catch future icu4c
bumps needed - At least, that's the hope. Pushing now.
Follow up to #39353. The test should ensure we catch the need to bump in future.
Process.kill("SIGINT", pid) | ||
Process.wait(pid) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
test do
begin
pid = fork do
exec "#{bin}/mpd --stdout --no-daemon --no-config"
end
sleep 2
assert_match /OK MPD/, shell_output("curl localhost:6600")
ensure
Process.kill("SIGINT", pid)
Process.wait(pid)
end
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seemingly, It can be either and works. Homebrew's internal use tends to prefer begin
inside the fork do
, formulae lean towards preferring the begin
after the fork do
😐.
Bot decided to crash on this last night. Green now. |
Thanks again @DomT4! |
Follow up to #39353. The test should ensure we catch the need to bump for
icu4c
in future.