Skip to content
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

testcheck: sort inline assertions #15290

Merged
merged 8 commits into from
May 24, 2023

Conversation

ikonst
Copy link
Contributor

@ikonst ikonst commented May 23, 2023

The order in which inline error assertions are collected into the TestCase.output (the "expected") can differ from the actual errors' order. Specifically, it's the order of the files themselves (as they're traversed during build), rather than individual lines.

To allow inline assertions across multiple modules, we can sort the collected errors to match the actual errors (only when it concerns module order; within the modules the order remains stable).

@ikonst ikonst force-pushed the sort-inline-assertions branch from c77b794 to a3e91a3 Compare May 23, 2023 17:43
@ikonst
Copy link
Contributor Author

ikonst commented May 23, 2023

@JelleZijlstra Let me know if it makes sense. I tried to avoid impacting performance by only doing it for the part of output that has inline assertions, and only for testcases having file sections.

(Also, doing it broadly for entire TestCase.output fails due to --pretty error lines not starting with "filename:" — kinda relates to #15273.)

@JelleZijlstra JelleZijlstra self-requested a review May 23, 2023 17:52
@@ -163,21 +176,20 @@ def run_case_once(
a = normalize_error_messages(a)

# Make sure error messages match
if incremental_step == 0:
if incremental_step < 2:
# Not incremental
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems outdated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if incremental_step == 1:
msg = "Unexpected type checker output in incremental, run 1 ({}, line {})"
else:
msg = "Unexpected type checker output ({}, line {})"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add assert incremental_step == 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -67,6 +67,7 @@ def parse_test_case(case: DataDrivenTestCase) -> None:
files: list[tuple[str, str]] = [] # path and contents
output_files: list[tuple[str, str | Pattern[str]]] = [] # output path and contents
output: list[str] = [] # Regular output errors
output_inline_start: int # Start index of output inline assertions
Copy link
Member

Choose a reason for hiding this comment

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

Don't think you need to declare this up front. The other variables are declared here so we can say what should be in the list/dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, right, right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JelleZijlstra JelleZijlstra merged commit 9270819 into python:master May 24, 2023
@ikonst ikonst deleted the sort-inline-assertions branch May 24, 2023 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants