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

gh-111147: Fix test_set_of_sets_reprs in test_pprint #111148

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Oct 21, 2023

I left @cpython_only for no real reason, I think that others should behave the same way.
But, it feels like a lower risk to me.

@vstinner
Copy link
Member

I suppose that @serhiy-storchaka would be a better candidate than me to review this change.

@serhiy-storchaka
Copy link
Member

I replaces tests with tests with deterministic result (when all sets can be strictly ordered). I do not know what to do with testing non-orderable sets, which perhaps was the purpose of the original set. Maybe we should test the actual result against multiple expected results.

@rhettinger rhettinger removed their request for review October 27, 2023 04:26
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I dislike skipped tests. IMO these tests are better than having tests expected to fail.

Lib/test/test_pprint.py Outdated Show resolved Hide resolved
@vstinner
Copy link
Member

@serhiy-storchaka: Are you ok with this change? Does it make test_pprint better?

Maybe we should test the actual result against multiple expected results.

Maybe add a test with a set of 2 strings and check if the repr() is in one of the two possible outputs?

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Nov 10, 2023

I thought that @rhettinger should make the decision, but he removed himself.

Well, I propose the following plan:

  1. Use strings instead of integers to make the iteration order and the repr non-deterministic.
  2. Test the result against all possible outcomes.
  3. To keep the number of variants (which grows exponentially) in reasonable limits, use sets with not more than 2 items.
  4. Most cases should be tested to produce single-line and multi-line results. To make the result multi-line, use smaller width value or longer strings, or both.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Actually, in the multiline case, the elements of the innermost frozensets are always sorted. So you can get

                    frozenset((
                        "qwerty is also absurdly long",
                        "xyz very-very long string",
                    )),

but not

                    frozenset((
                        "xyz very-very long string",
                        "qwerty is also absurdly long",
                    )),

It will reduce the number of cases by 4 times.

On other hand, in the single-line variant the result is the same as the repr. So pprint.pformat(...) == repr(...) if it fits in a single line.

There is an interesting middle case. When the result is multiline, but inner frozensets fit in a single line. Then there are just two variants, differentiated by the order of elements of the outer frozenset.

"""
frozenset({%r,
           %r})
""" % (f1, f2)

and

"""
frozenset({%r,
           %r})
""" % (f2, f1)

@sobolevn
Copy link
Member Author

@serhiy-storchaka

  1. Thanks, I was not familiar with this implementation detail:

Actually, in the multiline case, the elements of the innermost frozensets are always sorted.

I've tried locally with these cases removed and while true; do ./python.exe -m test test_pprint -m test_set_of_sets_reprs -v; done showed that this is actually true.

  1. I've also added repr case.
  2. Added the case for multiline result with single-line frozensets 👍

However, it does not work the same way as fully multiline case.

For example, this result is expected:

frozenset({frozenset({'regular string', 'other string'}),
           frozenset({'third string', 'one more string'})})

Traceback:

» ./python.exe -m test --forever test_pprint -m test_set_of_sets_reprs
Using random seed: 1266073671
0:00:00 load avg: 2.92 Run tests sequentially
0:00:00 load avg: 2.92 [  1] test_pprint
test test_pprint failed -- Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/Lib/test/test_pprint.py", line 695, in test_set_of_sets_reprs
    check(
  File "/Users/sobolev/Desktop/cpython2/Lib/test/test_pprint.py", line 692, in check
    self.assertIn(res, [textwrap.dedent(i).strip() for i in invariants])
AssertionError: "frozenset({frozenset({'regular string', 'other string'}),\n           frozenset({'third string', 'one more string'})})" not found in ["frozenset({frozenset({'other string', 'regular string'}),\n           frozenset({'third string', 'one more string'})})", "frozenset({frozenset({'other string', 'regular string'}),\n           frozenset({'one more string', 'third string'})})", "frozenset({frozenset({'third string', 'one more string'}),\n           frozenset({'other string', 'regular string'})})", "frozenset({frozenset({'third string', 'one more string'}),\n           frozenset({'regular string', 'other string'})})"]

Lib/test/test_pprint.py Outdated Show resolved Hide resolved
Lib/test/test_pprint.py Outdated Show resolved Hide resolved
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka serhiy-storchaka added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 16, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 6f8ca31 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 16, 2023
@serhiy-storchaka serhiy-storchaka merged commit 7ac49e7 into python:main Nov 27, 2023
98 of 105 checks passed
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…nGH-111148)

Make it stable and not depending on implementation details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants