Skip to content

gh-110932: Fix regrtest for SOURCE_DATE_EPOCH #111143

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

Merged
merged 1 commit into from
Oct 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions Lib/test/libregrtest/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,14 +129,19 @@ def __init__(self, ns: Namespace, _add_python_opts: bool = False):

# Randomize
self.randomize: bool = ns.randomize
self.random_seed: int | None = (
ns.random_seed
if ns.random_seed is not None
else random.getrandbits(32)
)
if 'SOURCE_DATE_EPOCH' in os.environ:
if ('SOURCE_DATE_EPOCH' in os.environ
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
if ('SOURCE_DATE_EPOCH' in os.environ
if (
'SOURCE_DATE_EPOCH' in os.environ

# don't use the variable if empty
and os.environ['SOURCE_DATE_EPOCH']
):
self.randomize = False
self.random_seed = None
# SOURCE_DATE_EPOCH should be an integer, but use a string to not
# fail if it's not integer. random.seed() accepts a string.
# https://reproducible-builds.org/docs/source-date-epoch/
self.random_seed: int | str = os.environ['SOURCE_DATE_EPOCH']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Myabe we should always use str? There's no real reason to use int here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is subtle. It avoids to import hashlib to convert a string to an integer in Random.seed(). Right now, hashlib is always imported, but I have a local branch to reduce the number of imports at Python startup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about the second code path, where we create a random seed as an integer.

elif ns.random_seed is None:
self.random_seed = random.getrandbits(32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, it would become:

Suggested change
self.random_seed = random.getrandbits(32)
self.random_seed = str(random.getrandbits(32))

else:
self.random_seed = ns.random_seed

# tests
self.first_runtests: RunTests | None = None
Expand Down Expand Up @@ -441,7 +446,7 @@ def _run_tests(self, selected: TestTuple, tests: TestList | None) -> int:
or tests or self.cmdline_args)):
display_header(self.use_resources, self.python_cmd)

print("Using random seed", self.random_seed)
print("Using random seed:", self.random_seed)

runtests = self.create_run_tests(selected)
self.first_runtests = runtests
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/libregrtest/runtests.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class RunTests:
use_resources: tuple[str, ...]
python_cmd: tuple[str, ...] | None
randomize: bool
random_seed: int | None
random_seed: int | str
json_file: JsonFile | None

def copy(self, **override):
Expand Down
72 changes: 59 additions & 13 deletions Lib/test/test_regrtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,18 +143,26 @@ def test_header(self):
self.assertTrue(ns.header)

def test_randomize(self):
for opt in '-r', '--randomize':
for opt in ('-r', '--randomize'):
with self.subTest(opt=opt):
ns = self.parse_args([opt])
self.assertTrue(ns.randomize)

with os_helper.EnvironmentVarGuard() as env:
env['SOURCE_DATE_EPOCH'] = '1'

# with SOURCE_DATE_EPOCH
env['SOURCE_DATE_EPOCH'] = '1697839080'
ns = self.parse_args(['--randomize'])
regrtest = main.Regrtest(ns)
self.assertFalse(regrtest.randomize)
self.assertIsNone(regrtest.random_seed)
self.assertIsInstance(regrtest.random_seed, str)
self.assertEqual(regrtest.random_seed, '1697839080')

# without SOURCE_DATE_EPOCH
del env['SOURCE_DATE_EPOCH']
ns = self.parse_args(['--randomize'])
regrtest = main.Regrtest(ns)
self.assertTrue(regrtest.randomize)
self.assertIsInstance(regrtest.random_seed, int)

def test_randseed(self):
ns = self.parse_args(['--randseed', '12345'])
Expand Down Expand Up @@ -388,7 +396,13 @@ def check_ci_mode(self, args, use_resources, rerun=True):

# Check Regrtest attributes which are more reliable than Namespace
# which has an unclear API
regrtest = main.Regrtest(ns)
with os_helper.EnvironmentVarGuard() as env:
# Ignore SOURCE_DATE_EPOCH env var if it's set
if 'SOURCE_DATE_EPOCH' in env:
del env['SOURCE_DATE_EPOCH']

regrtest = main.Regrtest(ns)

self.assertEqual(regrtest.num_workers, -1)
self.assertEqual(regrtest.want_rerun, rerun)
self.assertTrue(regrtest.randomize)
Expand Down Expand Up @@ -661,21 +675,26 @@ def list_regex(line_format, tests):
state = f'{state} then {new_state}'
self.check_line(output, f'Result: {state}', full=True)

def parse_random_seed(self, output):
match = self.regex_search(r'Using random seed ([0-9]+)', output)
randseed = int(match.group(1))
self.assertTrue(0 <= randseed, randseed)
return randseed
def parse_random_seed(self, output: str) -> str:
match = self.regex_search(r'Using random seed: (.*)', output)
return match.group(1)

def run_command(self, args, input=None, exitcode=0, **kw):
if not input:
input = ''
if 'stderr' not in kw:
kw['stderr'] = subprocess.STDOUT

env = kw.pop('env', None)
if env is None:
env = dict(os.environ)
env.pop('SOURCE_DATE_EPOCH', None)

proc = subprocess.run(args,
text=True,
input=input,
stdout=subprocess.PIPE,
env=env,
**kw)
if proc.returncode != exitcode:
msg = ("Command %s failed with exit code %s, but exit code %s expected!\n"
Expand Down Expand Up @@ -751,7 +770,9 @@ def setUp(self):
self.regrtest_args.append('-n')

def check_output(self, output):
self.parse_random_seed(output)
randseed = self.parse_random_seed(output)
self.assertTrue(randseed.isdigit(), randseed)

self.check_executed_tests(output, self.tests,
randomize=True, stats=len(self.tests))

Expand Down Expand Up @@ -942,7 +963,7 @@ def test_random(self):
test_random = int(match.group(1))

# try to reproduce with the random seed
output = self.run_tests('-r', '--randseed=%s' % randseed, test,
output = self.run_tests('-r', f'--randseed={randseed}', test,
exitcode=EXITCODE_NO_TESTS_RAN)
randseed2 = self.parse_random_seed(output)
self.assertEqual(randseed2, randseed)
Expand All @@ -953,7 +974,32 @@ def test_random(self):

# check that random.seed is used by default
output = self.run_tests(test, exitcode=EXITCODE_NO_TESTS_RAN)
self.assertIsInstance(self.parse_random_seed(output), int)
randseed = self.parse_random_seed(output)
self.assertTrue(randseed.isdigit(), randseed)

# check SOURCE_DATE_EPOCH (integer)
timestamp = '1697839080'
env = dict(os.environ, SOURCE_DATE_EPOCH=timestamp)
output = self.run_tests('-r', test, exitcode=EXITCODE_NO_TESTS_RAN,
env=env)
randseed = self.parse_random_seed(output)
self.assertEqual(randseed, timestamp)
self.check_line(output, 'TESTRANDOM: 520')

# check SOURCE_DATE_EPOCH (string)
env = dict(os.environ, SOURCE_DATE_EPOCH='XYZ')
output = self.run_tests('-r', test, exitcode=EXITCODE_NO_TESTS_RAN,
env=env)
randseed = self.parse_random_seed(output)
self.assertEqual(randseed, 'XYZ')
self.check_line(output, 'TESTRANDOM: 22')

# check SOURCE_DATE_EPOCH (empty string): ignore the env var
env = dict(os.environ, SOURCE_DATE_EPOCH='')
output = self.run_tests('-r', test, exitcode=EXITCODE_NO_TESTS_RAN,
env=env)
randseed = self.parse_random_seed(output)
self.assertTrue(randseed.isdigit(), randseed)

def test_fromfile(self):
# test --fromfile
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix regrtest if the ``SOURCE_DATE_EPOCH`` environment variable is defined:
use the variable value as the random seed. Patch by Victor Stinner.