Ticket #1951 (closed bug: fixed)

Opened 4 years ago

Last modified 4 years ago

t/dynpmc/os.t incorrectly uses system-specific error strings

Reported by: doughera Owned by: nwellnhof
Priority: normal Milestone:
Component: testing Version: 2.11.0
Severity: high Keywords: gci, cage
Cc: Language:
Patch status: Platform:

Description

On Solaris, t/dynpmc/os.t is failing tests 2 and 9 because the failure strings returned do not match the expected values. (I'm guessing they are the Linux return values). The output of the test is attached.

Attachments

tt1951-solaris.txt Download (1.5 KB) - added by doughera 4 years ago.
t/dynpmc/os.t failures

Change History

Changed 4 years ago by doughera

t/dynpmc/os.t failures

  Changed 4 years ago by doughera

This failure is still present in 3.0.0.

  Changed 4 years ago by dukeleto

  • keywords gci, cage added
  • component changed from none to testing
  • severity changed from medium to high

follow-up: ↓ 4   Changed 4 years ago by jkeenan

Would it be plausible to try to capture the system's own output of these (bad) commands and use that as the basis of the pattern which the test tries to match? That way, we wouldn't be trying to match a hard-coded string.

in reply to: ↑ 3   Changed 4 years ago by doughera

Replying to jkeenan:

Would it be plausible to try to capture the system's own output of these (bad) commands and use that as the basis of the pattern which the test tries to match? That way, we wouldn't be trying to match a hard-coded string.

I'm not sure it's important to test the exact error message in these cases. I'd think it would be sufficient to test that operations that should succeed do succeed, and that operations that should fail do fail.

  Changed 4 years ago by nwellnhof

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

I ran into that issue when converting the OS dynpmc to use the Win32 API on Windows. Should be fixed after I merge that work.

follow-up: ↓ 7   Changed 4 years ago by nwellnhof

Should be fixed after branch merge in 8b817771f1bc40691685

in reply to: ↑ 6   Changed 4 years ago by doughera

Replying to nwellnhof:

Should be fixed after branch merge in 8b817771f1bc40691685

Thanks. On Solaris 8, test 9 now passes, but test 2 still fails with:

not ok 2 - Test bad cwd

#   Failed test 'Test bad cwd'
#   at t/dynpmc/os.t line 82.
#                   'rmdir failed: Invalid argument
# current instr.: 'main' pc 22 (/net/hubble/export/sans3/doughera/src/parrot/parrot-git/t/dynpmc/os_2.pir:6)
# '
#     doesn't match '/getcwd failed/
# '
# './parrot   "/net/hubble/export/sans3/doughera/src/parrot/parrot-git/t/dynpmc/os_2.pir"' failed with exit code 1

That is, the rmdir command is failing, not the subsequent getcwd .

follow-up: ↓ 9   Changed 4 years ago by nwellnhof

According to the Solaris man page, rmdir returns EINVAL if the directory to be removed is the current directory, so this test should be skipped. See  http://download.oracle.com/docs/cd/E19109-01/tsolaris7/805-8068/6j7j917bt/index.html

in reply to: ↑ 8   Changed 4 years ago by doughera

Replying to nwellnhof:

According to the Solaris man page, rmdir returns EINVAL if the directory to be removed is the current directory, so this test should be skipped.

Is this test really testing something that parrot wishes to promise it can do? This test checks whether you can remove the current directory (provided you call it in just the right way -- note that rmdir . won't work on Linux either) and then get an error trying to getcwd(). Is that really behavior parrot wants to test for? If the test is simply skipped everywhere it doesn't work (such as Windows and Solaris, for starters), what does the passing test really prove?

More succinctly, I think the test is simply wrong and should be deleted.

  Changed 4 years ago by nwellnhof

The test tries to execute the failure code path of cwd. Removing the current directory seems to be the only way to make cwd fail, at least on some platforms.

follow-up: ↓ 12   Changed 4 years ago by doughera

Fair enough. Testing the failure code path of cwd is good. The bizarre strategy in the test seems to be a workaround for a Linux-specific "feature". On OpenBSD and Solaris, at least, changing the permissions (with chmod) is sufficient to get the getcwd() call to fail with EACCESS, as you'd expect.

To be defensive, I think the test should probably only be tried on systems where removing the current directory is known to be a valid operation. That is, it should probably be something like 'skip unless Linux'.

Thanks for the research and explanation.

in reply to: ↑ 11   Changed 4 years ago by nwellnhof

Replying to doughera:

To be defensive, I think the test should probably only be tried on systems where removing the current directory is known to be a valid operation. That is, it should probably be something like 'skip unless Linux'.

Makes sense. Changed in commit 0986ee349528b3d1b3e3.

  Changed 4 years ago by doughera

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

Works for me. Closing Ticket. Thanks.

Note: See TracTickets for help on using tickets.