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

Safe.pm test suite has some broken-looking skip conditions #23114

Open
mauke opened this issue Mar 15, 2025 · 1 comment
Open

Safe.pm test suite has some broken-looking skip conditions #23114

mauke opened this issue Mar 15, 2025 · 1 comment

Comments

@mauke
Copy link
Contributor

mauke commented Mar 15, 2025

Module: Safe

Description

Some of the tests in dist/Safe/t are conditional. Most of them just check whether $Config{extensions} lists Opcode as available, which makes sense since Safe uses Opcode. But a few of them add some extra conditions that make no sense to me.

The worst offender is probably dist/Safe/t/safe3.t:

BEGIN {
    require Config; Config->import;
    if ($Config{'extensions'} !~ /\bOpcode\b/
	&& $Config{'extensions'} !~ /\bPOSIX\b/
	&& $Config{'osname'} ne 'VMS')
    {
	print "1..0\n";
	exit 0;
    }
}

Essentially, what this is saying is:

  • On VMS, always run this test, regardless of whether Opcode/POSIX are available.
  • On non-VMS systems, run this test if either Opcode or POSIX are available.
  • Only skip this test if we're not on VMS and both Opcode and POSIX are unavailable.

I would've expected all those && to be ||, i.e. to skip the test if we're on VMS, or Opcode is unavailable, or POSIX is unavailable, because the following code then unconditionally loads POSIX and Safe (which pulls in Opcode).

The full list of test files with similar code is:

  • dist/Safe/t/safe1.t
  • dist/Safe/t/safe2.t
  • dist/Safe/t/safe3.t
  • dist/Safe/t/safeops.t
  • dist/Safe/t/safesort.t
  • dist/Safe/t/safeutf8.t
  • dist/Safe/t/safewrap.t

Steps to Reproduce

grep VMS dist/Safe/t/*

Expected behavior
The skip conditions should logically use ||, not &&.

@tonycoz
Copy link
Contributor

tonycoz commented Mar 17, 2025

I missed this when reviewing #23120.

I had a look over the history of some of the tests, my first suspicion was that VMS wasn't populating extensions at some point, but I didn't find any indication of that in the history of the VMS configure.com.

Maybe @craigberry has some idea for why the VMS check is done.

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

No branches or pull requests

3 participants