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

Add STDIN to tests #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add STDIN to tests #35

wants to merge 3 commits into from

Conversation

dylanstreb
Copy link

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
Copy link
Collaborator

jkeenan commented Mar 5, 2020

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
Copy link
Author

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
Copy link

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
Copy link
Author

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 1d99f77 . 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
Copy link

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
Copy link
Author

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 jkeenan mentioned this pull request Mar 15, 2020
@jkeenan
Copy link
Collaborator

jkeenan commented Mar 21, 2020

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
Copy link
Author

dylanstreb commented Mar 21, 2020

#   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.

jkeenan added a commit to jkeenan/ipc-system-simple that referenced this pull request Mar 22, 2020
Per discussion with dylanstreb in
pjf#35 and elsewhere, we'll
mark certain tests in t/win32.t to be run only under AUTHOR_TESTING.
These tests are passing on AppVeyor but failing on CPANtesters.  We'll
have to have Windows experts and Windows equipment to diagnose further.
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.

3 participants