Skip to content

Conversation

@mpage
Copy link
Contributor

@mpagempage commented Mar 6, 2025

Memory allocation ends up failing in _PyRefchainTrace(), which produces different output. Assert that we don't segfault, which is the thing we want to test and is less brittle than checking output.

Memory allocation ends up failing in _PyRefchainTrace(), which produces different output. Assert that we don't segfault, which is the thing we want to test and is less brittle than checking output.
@bedevere-appbedevere-appbot added the tests Tests in the Lib/test dir label Mar 6, 2025
@mpagempage marked this pull request as ready for review March 6, 2025 18:56
""")
_, _, err=assert_python_failure("-c", code)
self.assertIn("MemoryError", err.decode("utf-8"))
rc, _, _=assert_python_failure("-c", code)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark the test with @unittest.skipIf(support.Py_TRACE_REFS, 'cannot test Py_TRACE_REFS build') so that we don't run it under --with-trace-refs?

That's what we do in test_repl's test_no_memory (a different test with the same name).

Along those lines, I'm a bit confused because the PR refers to #118331, but that's a bug report for test_no_memory in test_repl.py not the test_no_memory in test_list.py.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Can we mark the test with @unittest.skipIf(support.Py_TRACE_REFS, 'cannot test Py_TRACE_REFS build') so that we don't run it under --with-trace-refs?

Sure, I have a slight preference for this change since it's testing the thing we care about (not segfaulting) and works in both builds, but that's fine with me.

Along those lines, I'm a bit confused because the PR refers to #118331, but that's a bug report for test_no_memory in test_repl.py not the test_no_memory in test_list.py.

The PR that introduced the failure under tracerefs is linked to that issue so I figured it was fine to link to it as well. I'll create a new issue for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay that makes sense. It doesn't need a new issue

@mpagempage merged commit 6c6600f into python:mainMar 6, 2025
48 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@mpage@colesbury