Locking %ENV during tests can make Test::More very unhappy.

[demerphq]

Restricted hashes have the downside that accessing a key they do not contain can cause them to throw a fatal exception. If you lock %ENV, and then run tests that fail (apparently not if they pass!) then Test2/Formatter/TAP.pm will throw its toys out of the pram.

I can debate this one a few ways. You could argue this a "doctor it hurts when i stick a fork in my eye" bug and that Test::More should just ignore it, another argument would be that Test::More should copy %ENV when it is used and only consult its copy, you could also argue that Test::More should test for key existence before accessing keys in %ENV. I think i lean towards the latter.

perl -MTest::More -MHash::Util=lock_hash -e'lock_hash(%ENV); is(1,2)'
Attempt to access disallowed key 'HARNESS_ACTIVE' in a restricted hash at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
	eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
	Test::Builder::ok(Test::Builder=HASH(0x56526e9811f8), "", undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 982
	Test::Builder::cmp_ok(Test::Builder=HASH(0x56526e9811f8), 1, "eq", 2, undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 820
	Test::Builder::is_eq(Test::Builder=HASH(0x56526e9811f8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
	Test::More::is(1, 2) called at -e line 1


Cleaning up the CONTEXT stack...
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
	eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
	Test::Builder::cmp_ok(Test::Builder=HASH(0x56526e9811f8), 1, "eq", 2, undef) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/Builder.pm line 820
	Test::Builder::is_eq(Test::Builder=HASH(0x56526e9811f8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
	Test::More::is(1, 2) called at -e line 1


Cleaning up the CONTEXT stack...
A context appears to have been destroyed without first calling release().
Based on $@ it does not look like an exception was thrown (this is not always
a reliable test)

This is a problem because the global error variables ($!, $@, and $?) will
not be restored. In addition some release callbacks will not work properly from
inside a DESTROY method.

Here are the context creation details, just in case a tool forgot to call
release():
  File: -e
  Line: 1
  Tool: Test::More::is

Here is a trace to the code that caused the context to be destroyed, this could
be an exit(), a goto, or simply the end of a scope:
Context destroyed at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
	eval {...} called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121
	Test::Builder::is_eq(Test::Builder=HASH(0x56526e9811f8), 1, 2) called at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test/More.pm line 405
	Test::More::is(1, 2) called at -e line 1


Cleaning up the CONTEXT stack...
Attempt to access disallowed key 'HARNESS_ACTIVE' in a restricted hash at /home/yorton/perl5/perlbrew/perls/perl-5.34.1/lib/site_perl/5.34.1/Test2/Formatter/TAP.pm line 121.
END failed--call queue aborted.

See Perl/perl5#20929 for the related perl ticket with information about how I discovered this issue.

[exodist]

I would like a few other toolchain dev's to weigh in on this before I decide to merge or ignore the patch. The patch seems harmless/fine, but I am also not sure this is right to add to Test::More, needing to treat %ENV different/special because some other module might lock it does feel out of scope. I also suspect there will be drift, anyone who submits PR's for Test-Simple in the future will have to know or find out that env vars need special treatment. Also how far do we go with this? Right now we are only concerned with Test::More, but should we apply the same consideration to all of Test2::Suite which is the next generation alternative to Test::More?

I do not want to just make the call without further input from other toolchain level developers.

[Leont]

needing to treat %ENV different/special because some other module might lock it does feel out of scope

Yeah, anything that messes with global scope in such a profound way is bound to break things, I don't think it makes sense to account for that.

[mohawk2]

needing to treat %ENV different/special because some other module might lock it does feel out of scope

Yeah, anything that messes with global scope in such a profound way is bound to break things, I don't think it makes sense to account for that.

I agree.

[demerphq]

needing to treat %ENV different/special because some other module might lock it does feel out of scope

I can live with that, but I think there is a flaw in your reasoning overall. I think you are seeing "locked %ENV" and thinking "wow that is exceptional, why should I guard against that!?", and at a certain level I would agree with you, locking %ENV is a weird thing to do. However if you look at it as "The test infra is depending on the consistency of a public data structure that the code I am testing might alter" then it sounds rather different don't you think? More like an accident waiting to happen, and it just happend that in this case it happened by locking %ENV. Consider what would happen if the code does local %ENV, which is a very reasonable thing for code that is being tested to do.

Note that you are reading from %ENV in Test2/Formatter/TAP.pm every failed test. I would argue that is canonically wrong. My patch was just a minimally invasive patch to reduce issues related to the very test case I found. But arguably you shouldn't be reading from %ENV during the lifetime of the tests at all. Once the code you are testing has started it might be using %ENV for its own purposes, and you shouldn't expect that you can depend on specific keys being present, or that its content will be consistent in value or composition over the lifetime of multiple tests. Id argue that is just plain and simply a design flaw. It seems reasonable to me to copy %ENV during the loading of the test module itself, or perhaps during some test constructor logic or something, but from that point on you shouldn't be using it at all. You should be using state that is completely private to the test infrastructure. Otherwise the test infrastructure cant be relied on to test code that alters %ENV. (Consider the implications of testing itself for instance.)

anyone who submits PR's for Test-Simple in the future will have to know or find out that env vars need special treatment

Personally I don't find that argument particularly persuasive. It would not be very difficult to test that you aren't reading from %ENV "in flight", in any number of ways, not least by including a test that empties %ENV and locks it and then runs a bunch of tests. :-)

should we apply the same consideration to all of Test2::Suite which is the next generation alternative to Test::More?

Depends what you mean. Should you guard against %ENV being locked by tests? Probably not. Instead you should be ensuring that any change to %ENV during the lifetime of the code you are testing does not alter the behavior of the test infrastructure. If the code you are testing might alter %ENV such that it breaks or changes the operation of the test infrastructure then it is a bug. Id argue that the test code should not depend on any public vars beyond the very initial setup of the test infra itself.

Consider the code that actually caused the problem here:

         print $io "\n"
            if $ENV{HARNESS_ACTIVE}
             && $hid == OUT_ERR
             && $self->{+_LAST_FH} != $io
             && $msg =~ m/^#\s*Failed( \(TODO\))? test /;

So this adds a newline if $ENV{HARNESS_ACTIVE}. So then what happens if the code you are testing does:

local %ENV; 
is(`$whatever`, $expected);

It wont print a newline apparently. Wouldn't you consider that a bug? Id expect that whatever uses $ENV{HARNESS_ACTION} sets up $Test::More::HARNESS_ACTIVE (or some other namespaced global) very early and then uses that instead?

Which brings us back to having tests that would ensure you don't use %ENV for anything once the test infra has been initialized. Which would be most naturally tested by locking %ENV. :-)

Anyway, I'm happy to close my PR. BUT

1. there are actual bugs fixed in it besides this env issue. (The formatter stuff.)
2. Arguably you shouldn't be reading from %ENV at all except for very very early in the process, and you could use the function I added to read from a private copy of %ENV made at startup.

Yeah, anything that messes with global scope in such a profound way is bound to break things, I don't think it makes sense to account for that.

I agree.

@Leont and @mohawk2 the natural conclusion of what you just said is that Test::More shouldn't be used to test code that does: local %ENV. I don't think that is a reasonable position. Lots of code empties out %ENV. For instance to ensure that %ENV variables can't propagate through to code that is spawned by the code being tested.

[exodist]

We for sure should fix the bug you found when parsing with a '+'

As for env vars being changed int he course of testing, there are scenarios on both sides. In many cases you are correct, we do not want behavior to change when env vars do. But in other we do. For instance the Test-Simple internal tests verify the behvaior changes for different values of $ENV{HARNESS_ACTIVE} and they do so by calling the sub twice, once with the env var set to 0, and one with it set to 1. IF we locked in the value at the begining, regardless of the mechanism, the test would have to be revised and would probably need to turn to IPC and call perl again internally with a different env var to see how it changes.

Given we can argue for either case, or find examples where either option makes some tests harder, I would prefer to take the side of doing less in the framework, and reducing complexity.

[mohawk2]

It seems like a good idea to make another PR fixing actual bugs like the + thing, as an immediate improvement.

[demerphq]

IF we locked in the value at the begining, regardless of the mechanism, the test would have to be revised and would probably need to turn to IPC and call perl again internally with a different env var to see how it changes.

That is what I would be doing, and what the core perl tests do all the time. We have 527 tests that use "fresh_perl_is" and "fresh_perl_like" for stuff like this. There are lots of things in core that can't be tested any other way. Many of the env vars that change how perl behaves are read exactly once, at startup, specifically because the code that perl executes might want to change (or lock :-) %ENV during the lifetime of the perl program, and because changing those things in mid flight can and will cause massive breakage.

Consider what would happen if perl read $ENV{PERL_HASH_SEED} every time it hashed a key. Leaving aside the chicken and egg issue, and the performance issue, if a user could change that value, then all hell would break loose. I don't understand why you would consider the test infra to be any different in this regard.

Given we can argue for either case, or find examples where either option makes some tests harder, I would prefer to take the side of doing less in the framework, and reducing complexity.

Well, to me expecting %ENV (or any shared global var, eg %SIG) to not be changed by the code you are testing is simply wrong. Code being tested absolutely will, from time to time, alter %ENV or empty or lock %ENV. It is a key control mechanism for IPC in terms of passing information to sub processes. Assuming otherwise seems to me to be just asking for trouble. If you are going to take this perspective then I would expect a large warning in the documentation for the test modules that test code must be proactive to ensure that any changes to %ENV by the code being tested must be guarded against, and frankly it seems such an unreasonable constraint on general purpose test code that I can't imagine how it would be worded.

I really respect the work you guys do on this, and i know its a real pain, but honestly on this subject I can't understand your position on this. Although I totally agree that my initial patch was wrong, the real bug is using %ENV in flight, not that you aren't guarding for it being locked.

It seems like a good idea to make another PR fixing actual bugs like the + thing, as an immediate improvement.

I'll let you guys pick this up. I will close my patch now, and you can do as you wish. I really don't mean to be passive aggressive about this, but i have other stuff cooking for core dev that can/will only be worked on by me, and there are at least two of you who can rip apart my patch sequence and extract whatever remains that you think is worth preserving.

[Leont]

Well, to me expecting %ENV (or any shared global var, eg %SIG) to not be changed by the code you are testing is simply wrong.

To me there's a big difference between changing a piece of %ENV, and changing all of it. The first is entirely reasonable to me, the second not so much (and incidentally, local %ENV isn't even legal on VMS if I recall correctly, for reasons you really don't want to know).