pjf / ipc-system-simple

Perl module to make running system commands and capturing errors as simple as possible.
http://search.cpan.org/perldoc?IPC::System::Simple
Other
19 stars 20 forks source link

Add STDIN to tests #35

Open dylanstreb opened 4 years ago

dylanstreb commented 4 years ago

Pass a Perl script (fail_test.pl) through STDIN to each invocation of Perl within the t files. If Perl is called without arguments, it will execute this script and simply print "Test failed.". Without STDIN, it will instead wait for input, which looks like the test hangs.

This doesn't fix issue #32 but it does make the failures more graceful.

I've only tested this on Windows.

jkeenan commented 4 years ago

Pass a Perl script (fail_test.pl) through STDIN to each invocation of Perl within the t files. If Perl is called without arguments, it will execute this script and simply print "Test failed.". Without STDIN, it will instead wait for input, which looks like the test hangs.

This doesn't fix issue #32 but it does make the failures more graceful.

I've only tested this on Windows.

Classify me as very skeptical of this approach. Seems like way overkill.

Thank you very much. Jim Keenan

dylanstreb commented 4 years ago

I did some more experimentation and it's enough to just close STDIN (and re-open as '/dev/null' to avoid warnings) at the top of each t file. This causes Perl to immediately exit with no output if called with no arguments. Would that be a better approach?

Again, that change works on Windows but that's all I've tested it on.

PhilterPaper commented 4 years ago

So, "Perl being called without arguments" (is that actually the test program being called without args?) is the cause of the hang? Should we be concentrating on why the arguments are missing, and test for/trap those cases, rather than trying to simply paint over the current broken test system? At best, preventing a hang (but still failing) should only be a temporary measure to keep the tests moving along, but long term it sounds like the tests need some fixing.

dylanstreb commented 4 years ago

Correct, that is why it's hanging. It's waiting for input and not receiving any. Manually sending EOF during the test causes the Perl interpreter to exit, but this needs to be done several times since there's several busted tests.

I don't consider this painting over the system; with this patch, the tests fail (as they should). This is just to make the failures more graceful, and not look like a system hang. I was hoping the Perl script in STDIN could be modified to print the program name, but after submitting this I took a quick look at the Perl source and argv[0] doesn't appear to be exposed on Windows. I don't think there's actually any advantage in this approach as opposed to just closing STDIN.

The cause of the test failures is 1d99f777afccb11a5c735325f78d2034a39e5e0f . This method of quoting breaks the single argument version of capture, so the (first) test invokes a program named "perl.exe output.pl", with no arguments. There's already a different pull request that fixes this issue and uses Win32::ShellQuote to handle escaping.

PhilterPaper commented 4 years ago

I would say that the test is failing because Windows seems to behave a bit differently than Linux/BSD, and the test shouldn't fail (in the sense of a false positive/false negative/breakdown). That the test shows that something didn't work right, when it was expected NOT to work right, should not result in the test blowing up as it does. I think the 04_capture test was supposed to work, so this is a basic test failure, not even a demonstration of something that's supposed to fail going awry. If the test behaves differently on Windows, there is something very wrong with the whole structure of the test! IPC::System::Simple itself may even have a fatal architectural flaw.

As I pointed out over in #32, I find it odd that not one automated tester has yet tested this on Windows, yet there appears to be nothing in Makefile.PL that excludes Windows. I'm wondering if everyone attempting to test on Windows is encountering this hang and thus ends up reporting nothing for Windows tests. That is something indicating a problem with CPAN that there's no way to capture these tests as failures on Windows.

dylanstreb commented 4 years ago

The lack of automated testing was due to the hidden dependency on Win32::Process. pr #30 should fix that. I'm not familiar with how CPAN testing works but I believe the hang is an unrelated issue. I recall seeing a CPAN testers output showing the missing dependency, but now I can't find it.

Correct, this test is supposed to work. The following test in 04_capture compares the output of capture() to qx() to make sure they're the same. Both capture('perl.exe output.pl') and qx('perl.exe output.pl') are supposed to run perl.exe with a single argument, and that's not happening. There's already a pull request to fix the broken Windows behavior; this one is just to make sure that if this issue pops up again, it doesn't cause the tests to hang.

jkeenan commented 4 years ago

I have released version 1.28_001 to CPAN. Before releasing it, I got a nice PASS on AppVeyor: https://ci.appveyor.com/project/jkeenan/ipc-system-simple/builds/31625675

This version eliminates the Dist::Zilla meta-structure in favor of good old-fashioned Makefile.PL.

However, it has failed the only Win32 CPANtesters report received so far: http://www.cpantesters.org/cpan/report/f63b2f96-6bf3-1014-85d4-fa3e27bc9f39

Could you try it out? You can download a tarball from here.

Thank you very much. Jim Keenan

dylanstreb commented 4 years ago
#   Failed test 'command with spaces should not error (capturex multi)'
#   at t/win32.t line 136.
#          got: '"dir with spaces\hello.exe" failed to start: "The system cannot find the path specified" at t/win32.t line 135.
# '
#     expected: ''

Exact same result. dir with spaces/hello.exe doesn't exist in the distribution so this isn't surprising.

I would just mark those tests as author only.