gh-128438: Add EnvironmentVarGuard for test_urllib, test_urllib2.py and urllib2_localnet.py

[WolframAlph]

• Issue: Use EnvironmentVarGuard in tests instead of manually messing with os.environ #128438

[picnixz]

The tests that use os.environ are always copying the current environment and pass it explicitly to subprocess.run(), which IMO is fine. We're not messing with the global environment since we're always using a fresh copy so I'm not sure it's needed.

If you find other tests that mess with os.environ directly and not a copy thereof, I'd be willing to consider such changes but I don't think this one is worth.

[WolframAlph]

The tests that use os.environ are always copying the current environment and pass it explicitly to subprocess.run(), which IMO is fine. We're not messing with the global environment since we're always using a fresh copy so I'm not sure it's needed.

👍 , feel free to close this one. Sorry for noise

[picnixz]

No worries! For instance test_open_default_encoding tweaks os.environ directly but this could use EnvironmentVarGuard. WDYT about checking other places like these ones? (you can just update this PR and re-use the same issue with a different title)

[WolframAlph]

Would it be fine to update all those places across multiple files in a single PR then?

[picnixz]

Other tests such as ProxyAuthTests in test_urllib2_localnet uses this kind of construction with a setUp/teardown (instead, you could use self.environ = EnvironmentVarGuard(); self.enter_context(self.environ) in the setUp() method instead).

[picnixz]

Would it be fine to update all those places across multiple files in a single PR then?

If there are not a lot of files and the changes are not too large, I'd say yes. If there are a lot of files, I'd prefer multiple PRs but one issue.

[WolframAlph]

If there are not a lot of files and the changes are not too large, I'd say yes. If there are a lot of files, I'd prefer multiple PRs but one issue.

👍 , will look into that.

[WolframAlph]

Updated issue title to reflect what is going to be done

[picnixz]

I'm converting this one to a draft until you commit what you found (so that reviewers are not attracted to it)

[WolframAlph]

So I have spent some time on this and identified files worth looking into:

test_urllib.py
test_builtin.py
test_io.py
test_locale.py
test_pdb.py
test_sysconfig.py
datetimetester.py
test_urllib2.py
test_urllib2_localnet.py
test_zoneinfo.py
test_runner.py
Lib/test/support/init.py
test_strptime.py

Would you like to have PR for each or all together? From what I've seen there are quite a few places to change so probably split them into separate PRs? Maybe couple files per each one? WDYT?

[picnixz]

Let's have one PR per file and re-use the same issue. If the changes are identical (namely we change the same env var for the same reasons but in multiple files) but is spread across files, you can keep them in one PR. Otherwise one PR per file.

For instance, the urllib2 tests can be grouped in one PR

[picnixz]

This definitely looks better!

[picnixz]

On Windows, it appears that the NO_PROXY variable is still there. Is it because it's in all caps?

[WolframAlph]

Im guessing here (I dont have windows system to reproduce and test) but IIRC environment variables on Windows are not case sensitive. Setting value in os.environ internally will convert to uppercase, FTR:

cpython/Lib/os.py

Lines 777 to 778 in 513a4ef

	def encodekey(key):
	return encode(key).upper()

.

So when EnvironmentVarGuard sets no_proxy and then No_Proxy, it basically sets NO_PROXY 2 times but internally it has 2 changed records of no_proxy and No_Proxy which can then mess things up on exit since EnvironmentVarGuard is not case aware. I can try to make commit to test it here to confirm, but we should probably just skip tests with environment variable case sensitivity on Windows anyway.

[WolframAlph]

Okay, seems like that did not help. This time there is no env diff reported though. Anyway we should probably just skip it on Windows? But it used to pass earlier which makes me wonder if it used to pass on accident or it was designed to work on Windows too.

[picnixz]

Mmh, maybe something else is happening. We need to investigate first (I don't have time today sorry :() and maybe ask some Windows experts.

[WolframAlph]

Okay so I guess I found the issue. If we look back at how environ was mocked:

cpython/Lib/test/test_urllib.py

Line 263 in 513a4ef

os.environ = collections.OrderedDict()

It was never behaving as proper environ, so it was case sensitive all the time, therefore it passed on Windows. Now we switched to proper environ and therefore case insensitivity breaks this test. So basically this test is pointless on Windows as there is no case sensitivity. BTW, my latest test commit proves that EnvironmentVarGuard does not handle env vars on Windows properly as there is no environ diff report. So its 2 separate issues. So:

1. We leave it as it was
2. We skip it on Windows

WDYT?

[picnixz]

I don't really know. I'm more comfortable with leaving it as is but let's ask @zooba for Windows-related stuff and @Eclips4 for tests related stuff.