IDLE: Don't touch .idlerc when testing.

[mlouielu]

BPO	30869
Nosy	@terryjreedy, @vstinner, @mlouielu
PRs	bpo-30869: regrtest: Add .idlerc to saved_test_environment #2614
Dependencies	bpo-8231: Unable to run IDLE without write-access to home directory bpo-15862: IDLE: startup problem when HOME does not exist

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = None
created_at = <Date 2017-07-07.03:21:06.107>
labels = ['3.8', 'expert-IDLE', 'type-bug', '3.7']
title = "IDLE: Don't touch .idlerc when testing."
updated_at = <Date 2019-03-20.19:35:54.948>
user = 'https://github.com/mlouielu'

bugs.python.org fields:

activity = <Date 2019-03-20.19:35:54.948>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = False
closed_date = None
closer = None
components = ['IDLE']
creation = <Date 2017-07-07.03:21:06.107>
creator = 'louielu'
dependencies = ['8231', '15862']
files = []
hgrepos = []
issue_num = 30869
keywords = []
message_count = 7.0
messages = ['297856', '297858', '297874', '297875', '297917', '297985', '338506']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'vstinner', 'louielu']
pr_nums = ['2614']
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue30869'
versions = ['Python 3.7', 'Python 3.8']

[mlouielu]

In bpo bpo-30780, there is a mistake of tearDownModule didn't restore the use rCfg.

To prevent future mistake, Adding .idlerc to regrtest saved_test_environment, so that --fail-env-changed option can detect .idlerc been changed in IDLE test.

[terryjreedy]

I would have liked the mistake in bpo-30780 to have been noticed, but I don't think the patch would have caught it. What I failed to restore was an in memory date structure that is part of the config module. The patch seems to be concerned only with the filesystem, which is something different.

On IDLE startup, config tries 1. to find a home directory, 2. within that a .idlerc directory, and 3. within that user config files. If 1. fails, it used getcwd() as home. Either way, if 2. fails, config tries to make .idlerc. Failure here is currently fatal, though I would like to eventually change that. So I presume creation succeeds at least once on each buildbot. Creating .idlerc is intentional behavior, not a mistake, and in the absence of buildbot owners complaining, I don't think it should be made a test failure. Creating non-empty user config files during a test (on user machines) would be a mistake, and I have made sure that tests do not touch them. Checking for user config changes within .idlerc would be okay,

Victor, it is up to you as regrtest expert to review both the implied policy change and the effect of the code change.

[vstinner]

I dislike the idea of opening all $HOME/.idlerc/ directory and store their content in memory.

If a unit test writes into $HOME, we already have an issue. Unit tests must not modify the user configuration. If an unit test is interrupted, there is a high risk of leaving the modified configuration.

If idle tests need a well defined configuration, I suggest to start by modifiying idlelib.config.idleConf to use a temporary directory, or mock the whole object.

[vstinner]

test_site has a similar issue: bpo-30227, "test_site must not write outside the build directory: must not write into $HOME/.local/".

[terryjreedy]

The current tests do not write anything. But IDLE currently requires the existence of .idlerc to run, and tries to write it if not found. Changing this requirement without disabling multiple features would not be trivial. On possibility might be to replace builtin open with a wrapper that returned named StringIO objects when the path began with '.idlerc/'. The StringIO would have to be cached so that closing and reopening the same path got the same object. But this is only good for one process. See below.

There are two existing issues, bpo-8231 and bpo-15862, about HOME not being writable and not existing. The first has a patch I wrote 2 years ago trying to do what you suggest above. It used tempfile.mkdtemp for 2.7 compatibility, by I would now use TemporaryDirectory. The discussion ended at that time with some unresolved issues.

One in particular is that IDLE runs with two processes, with the user execution process being based on run.py. One of the idlelib modules imported by run eventually imports config. See msg253681 and msg253714.

The is already a mechanism for test_idle to tell idlelib module 'testing'. If and when bpo-8231 is solved, that mechanism could be used to ignore $HOME.

[terryjreedy]

bpo-27534 is about reducing run's imports. I believe that the current patch may remove one path by which config is indirectly imported. While writing msg297983 of bpo-30868, I remembered that config files are not the only files put in .idlerc. So a solution to block writing config files while testing is not enough. An in-memory .idlerc with StringIOs for files, both for testing and for user systems without a place to write it, is looking more inviting to me than it did yesterday.

[terryjreedy]

I closed PR 2614 in response to Victor's comments, in particular his preference that tests not even read .idlerc. Since we do not want tests to depend on the contents, there is no need to even read the contents. (And the exception of testing write-read round-tripping requires a fresh temporary directory.)

I propose that when testing, config.py should create idleConf.usercfg empty, without reference to .idlerc. The result would be the same as now after usercfg is replaced with testcfg, as in test_configdialog, without the need to revert the replacement.

test_idle has the equivalent of
import idlelib
idlelib.testing = True
The same should be added to any test module that sets options. Then idlelib.testing can be used to change config.py behavior.

Victor: what changes to a module trigger the 'environment changed' warning/error? The above unreverted change to idlelib.__init__ does not. The unreverted change to idleCong in test_configdialog did, until fixed.