Skip to content

gh-136438: Make sure test_generated_cases pass with all optimization levels - #136594

Merged
Eclips4 merged 2 commits into
python:mainfrom
efimov-mikhail:issue-136438-test_generated_cases
Jul 21, 2025
Merged

gh-136438: Make sure test_generated_cases pass with all optimization levels#136594
Eclips4 merged 2 commits into
python:mainfrom
efimov-mikhail:issue-136438-test_generated_cases

Conversation

@efimov-mikhail

@efimov-mikhailefimov-mikhail commented Jul 12, 2025

Copy link
Copy Markdown
Member

Now tests pass with all combinations of -OO and --without-doc-strings.

Before:

======================================================================
FAIL: test_missing_override_failure (test.test_generated_cases.TestGeneratedAbstractCases.test_missing_override_failure)
----------------------------------------------------------------------
AssertionError: 'case OP: {\n JitOptRef out;\n [104 chars] }' != ''
- case OP: {
- JitOptRef out;
- out = sym_new_not_null(ctx);
- stack_pointer[-1] = out;
- break;
- }
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/sikko/projects/cpython/Lib/test/test_generated_cases.py", line 2040, in test_missing_override_failure
with self.assertRaisesRegex(AssertionError, "All abstract uops"):
~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: "All abstract uops" does not match "'case OP: {\n JitOptRef out;\n [104 chars] }' != ''
- case OP: {
- JitOptRef out;
- out = sym_new_not_null(ctx);
- stack_pointer[-1] = out;
- break;
- }
"

@bedevere-appbedevere-appBot mentioned this pull request Jul 12, 2025
8 tasks
@efimov-mikhailefimov-mikhail changed the title gh-136438: Make sure test_generated_cases pass with all optimization levelsgh-136438: Make sure test_generated_cases pass with all optimization levelsJul 12, 2025
@efimov-mikhail

Copy link
Copy Markdown
MemberAuthor

Cc @Eclips4

@Eclips4
Eclips4 self-requested a review July 19, 2025 09:18
Comment threadTools/cases_generator/optimizer_generator.py
abstract_uop_name in base_uop_names
), f"All abstract uops should override base uops, but {abstract_uop_name} is not."
if abstract_uop_name not in base_uop_names:
raise AssertionError(f"All abstract uops should override base uops, "

@Eclips4Eclips4Jul 19, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Though I'm not sure, from an architectural point of view, that raising AssertionError manually is appropriate. I think we should change it to ValueError and handle it in the tests. WDYM?

@efimov-mikhailefimov-mikhailJul 19, 2025

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm agree that raising ValueError would be better. My first attempt was just to make as few changes to code as possible.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess it's ok to change it in this PR. Catching an AssertionError is semantically incorrect :)

@Eclips4
Eclips4 merged commit 58d305c into python:mainJul 21, 2025
48 checks passed
@Eclips4

Copy link
Copy Markdown
Member

Thank you Mikhail.

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
…ization levels (python#136594)
Fix the `test_generated_cases` to work with `-O` or `-OO` flags.
Previously, `test_generated_cases` was catching an `AssertionError` while `Tools/cases_generator/optimizer_generator.py` used an `assert` statement. This approach semantically incorrect, no one should trying to catch an `AssertionError`!
Now the `assert` statement has been replaced with an explicit `raise ValueError(...)` and the corresponding `self.assertRaisesRegex(AssertionError, ...)` has been updated to catch a `ValueError` instead.
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
…ization levels (python#136594)
Fix the `test_generated_cases` to work with `-O` or `-OO` flags.
Previously, `test_generated_cases` was catching an `AssertionError` while `Tools/cases_generator/optimizer_generator.py` used an `assert` statement. This approach semantically incorrect, no one should trying to catch an `AssertionError`!
Now the `assert` statement has been replaced with an explicit `raise ValueError(...)` and the corresponding `self.assertRaisesRegex(AssertionError, ...)` has been updated to catch a `ValueError` instead.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@efimov-mikhail@Eclips4@StanFromIreland