Ticket #840 (closed bug: fixed)

Opened 5 years ago

Last modified 3 years ago

t/op/io.t fails on win32

Reported by: coke Owned by: jkeenan
Priority: normal Milestone:
Component: testing Version: 1.4.0
Severity: release Keywords:
Cc: Language:
Patch status: Platform: win32

Description

> prove -v t\op\io.t
t\op\io.t ..
1..4
ok 1 - open with null filename
ok 2 - open with null mode
ok 3 - open pipe for reading
Failed 1/4 subtests

Test Summary Report
-------------------
t\op\io.t (Wstat: 0 Tests: 3 Failed: 0)
  Parse errors: Bad plan.  You planned 4 tests but ran 3.
Files=1, Tests=3,  0 wallclock secs ( 0.05 usr +  0.00 sys =  0.05 CPU)
Result: FAIL

Attachments

win32_pipe_write_stdhandles.patch Download (397 bytes) - added by flh 5 years ago.

Change History

  Changed 5 years ago by fperrad

This issue is not in the pipe implementation, but in the test file t/op/io.t

This issue starts with r39804 : the PIR conversion of t/op/io.t

  Changed 5 years ago by coke

  • owner set to coke

  Changed 5 years ago by jkeenan

  • component changed from none to core

  Changed 5 years ago by coke

  • status changed from new to assigned
  • version changed from trunk to 1.4.0
  • component changed from core to testing
  • milestone changed from 1.4 to 1.5

This is still failing in trunk post-release, so I'm assuming it went out like this.

Since this bug started with the conversion of the test, a patch to switch this test back to perl (instead of PIR) but keeping changes after r39804 would be appreciated.

  Changed 5 years ago by coke

  • status changed from assigned to new
  • owner coke deleted

  Changed 5 years ago by flh

I'm really starting to think that this is a bug in the current implementation of pipes under Win32:

  • I took the previous (i.e. Perl) version of the test. The test passes, and generates a file t/op/io_4.pir corresponding to the pipe write part. Yet, directly running parrot t/op/io_4.t doesn't give the expected output,
  • funny thing: running perl -e "prove -v t\op\io.t" makes the test pass. That's why the Perl variant of io.t passes while the PIR variant doesn't,
  • and final thing: look at src/io/win32.c, line 688. This sets the child's stdin when we open a pipe for writing. What about stdout and stderr?

Applying the attached patch, which explicitly sets these two, makes the test pass. But, to be honest, I don't know Windows' API at all, so this patch could be wrong in some cases (don't we need to call DuplicateHandle instead of passing handles directly?).

Now the remaining question could be: does someone know which magic Perl uses to have stdhandles automagically inherited? Maybe this would lead to a better patch to fix this.

Changed 5 years ago by flh

  Changed 5 years ago by NotFound

The test is bad (I know, I wrote it). If we want to change the pipe implementation we must have a better reason than to make a bad test pass.

The test depends on the standard output of a pipe'd command, and we don't have a documented requirement for that.

I wrote the test that way just because it was the quickest way at the moment, but what this test must check is the writing on the pipe, not whatever happens to the stdout of the piped command. That part must be the matter of other test, if and when we have a documented behavior for that.

So the correct fix now is to replace this test with a better one, not changing parrot because of an implementation detail of a bad test. Maybe the pipe implementation must be changed, but not for such a poor reason.

follow-up: ↓ 10   Changed 5 years ago by whiteknight

There is a proper pipe implementation being developed in the io_cleanups branch. I will make sure this issue is resolved there before merging back into trunk. Expect progress over the weekend.

  Changed 5 years ago by whiteknight

  • milestone 1.5 deleted

in reply to: ↑ 8   Changed 5 years ago by jkeenan

Replying to whiteknight:

There is a proper pipe implementation being developed in the io_cleanups branch.

Was this indeed resolved when the io_cleanups branch was merged (or deleted, I don't know which)?

Thank you very much.

kid51

follow-up: ↓ 12   Changed 5 years ago by whiteknight

The io_cleanups branch was deleted without merging. I have a patch still, but the work is incomplete and waiting for input from Infinoid.

in reply to: ↑ 11   Changed 5 years ago by jkeenan

Replying to whiteknight:

The io_cleanups branch was deleted without merging. I have a patch still, but the work is incomplete and waiting for input from Infinoid.

Noting that the situation remains the same as of r41395, Sep 21 2009. See, for example,  this Smolder report from our current lone Win32 smoke tester.

kid51

  Changed 4 years ago by nwellnhof

  • platform set to win32

follow-up: ↓ 16   Changed 3 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned

t/op/io.t no longer is found in our distribution. Closing ticket.

$ ll t/op/*.t | grep io
-rw-r--r-- 1 jimk jimk 12550 Jan 23 22:59 t/op/exceptions.t

  Changed 3 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to invalid

in reply to: ↑ 14 ; follow-up: ↓ 17   Changed 3 years ago by coke

  • status changed from closed to reopened
  • resolution invalid deleted

Replying to jkeenan:

t/op/io.t no longer is found in our distribution. Closing ticket. {{{ $ ll t/op/*.t | grep io -rw-r--r-- 1 jimk jimk 12550 Jan 23 22:59 t/op/exceptions.t }}}

It's dynoplibs/io.t these days. I can't tell you if it's still failing, though.

in reply to: ↑ 16   Changed 3 years ago by jkeenan

Replying to coke:

It's dynoplibs/io.t these days. I can't tell you if it's still failing, though.

Based on the Smolder reports on Win32 we're getting today, I'd say the tests are no longer failing and that we can re-close this ticket.

Agreed?

Thank you very much.

kid51

  Changed 3 years ago by jkeenan

  • status changed from reopened to closed
  • resolution set to fixed

Via IRC #parrot, Coke indicated satisfaction with state of ticket. Closing.

Note: See TracTickets for help on using tickets.