bpo-40058: Fix failed test_divide_and_round of test_datetime when run more than … by shihai1991 · Pull Request #19213 · python/cpython

@vstinner @shihai1991 I don't think we can rely on "the tests pass" or "the problem in bpo-40058 is fixed" to determine whether or not this is a good change, because you're removing code that is designed to ensure that datetime.datetime is in a clean state after the import trickery happens.

The other XXX comment hints at a potential problem:

# XXX(gb) First run all the _Pure tests, then all the _Fast tests.  You might
# not believe this, but in spite of all the sys.modules trickery running a _Pure
# test last will leave a mix of pure and native datetime stuff lying around.

I think the questions to answer before we can remove this (per Chesterton's Fence) are:

1. Why was it deemed necessary to do this in the first place?
2. Has anything changed that would invalidate the reasoning for #⁠1?
3. Why was it having any effect at all (considering it's intended to keep the sys.modules cache clean, in which case subsequent import statements should just pull in these modules again)?

because you're removing code that is designed to ensure that datetime.datetime is in a clean state after the import trickery happens.

To be honest, I don't know the purpose of clean state of sys.modules :(

@vstinner @shihai1991 I don't think we can rely on "the tests pass" or "the problem in bpo-40058 is fixed" to determine whether or not this is a good change, because you're removing code that is designed to ensure that datetime.datetime is in a clean state after the import trickery happens.

import_fresh_module() saves/restores datetime, _datetime and _strptime modules of sys.modules. I don't understand what do you mean by "clean state"? The PR removes:

    # XXX: import_fresh_module() is supposed to leave sys.module cache untouched,
    # XXX: but it does not, so we have to cleanup ourselves.

I expect that import_fresh_module() works as expected. If it's not the case, it should be fixed. But test_datetime must not workaround bugs, if there are bugs.

@vstinner I agree that the final solution to this problem should be to fix the underlying problem and remove the workarounds, but we can't remove the workarounds until we know what they are working around - that is essentially tantamount to breaking the tests - and what's worse, we don't even know how we're breaking them - the tests still pass, but they may be testing something other than they are expected to test.

Again this is a perfect Chesterton's Fence situation: I am eager to remove the "fence", but only once I know why it's here.

Hm, I should know the Historical reason:
the original patch is: https://bugs.python.org/file18155/issue7989e.diff
in cf86e36(after this patch merged in):

>>> import sys
>>> from test.support import import_fresh_module, run_unittest
>>> saved_sys_modules = sys.modules.copy()
>>> len(sys.modules)
97
>>> pure_tests = import_fresh_module(TESTS, fresh=['datetime', '_strptime'], blocked=['_datetime'])
>>> fast_tests = import_fresh_module(TESTS, fresh=['datetime', '_datetime', '_strptime'])
>>> len(sys.modules)
103
>>> for module in sys.modules:
...     if module not in saved_sys_modules:
...         print(module)
...
datetime
test.datetimetester
calendar
math
_datetime
_strptime

so it increased 6 modules in sys.modules(in master, it would be more).
so there is another question is: datetime、_datetime、striptime should be removed after calling
import_fresh_module()?

victor's #20472 will fix this bug, so I close this PR.
Thanks, folks.

I think #20472 incidentally fixes this issue. If anyone can figure out why this stuff was added in the first place, there still might be good reason to remove it. TBH, there might be even more reason to remove it after #20472, because the sys.modules modifications are almost certainly related to the fact that import datetime creates a bunch of stuff and then deletes it later, which is no longer happening AFAICT.