bpo-42109: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests #22863

pganssle · 2020-10-21T17:43:41Z

This PR adds the property tests for zoneinfo currently maintained at https://github.com/Zac-HD/stdlib-property-tests, along with a compatibility interface for hypothesis that gracefully degrades to simple parameterized tests.

I haven't stubbed out the entire interface, but I have stubbed out a bit more than I am using as part of the zoneinfo tests right now (mostly the subset that I saw in use in the other stdlib-property-tests). We can scale down the stubbed interface if we want, and then have people add in the parts they need as it comes up. I don't think it's a good idea to proactively stub out the entire hypothesis interface, since those things would need maintenance over time.

If we like this approach, I will also add in some tooling to run the tests with hypothesis, for use in a buildbot and/or optional PR job.

CC: @Zac-HD, @cfbolz

https://bugs.python.org/issue42109

Zac-HD

Looks great - I'm very excited to get this running 😁

Lib/test/support/_hypothesis_stubs/__init__.py

github-actions · 2020-12-16T00:28:24Z

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

Lib/test/support/_hypothesis_stubs/__init__.py

These are stubs to be used for adding hypothesis (https://hypothesis.readthedocs.io/en/latest/) tests to the standard library. When the tests are run in an environment where `hypothesis` and its various dependencies are not installed, the stubs will turn any tests with examples into simple parameterized tests and any tests without examples are skipped.

This migrates the tests from https://github.com/Zac-HD/stdlib-property-tests into the standard library, using the hypothesis stubs.

pganssle · 2021-05-13T14:52:47Z

Lib/test/test_zoneinfo/test_zoneinfo_property.py

+    raise unittest.SkipTest("No time zone data available")
+
+
+def valid_keys():


These tests were implemented before this, and the latest version of hypothesis has a .timezone_keys strategy as well as a .timezones strategy, so we should probably refactor this, but there's no rush.

terryjreedy

2 PRs in 1. Do you consider the hypothesis_helper part to be finished and ready to merge?

I intend to add more on the issue later.

Lib/test/support/_hypothesis_stubs/__init__.py

terryjreedy · 2021-05-25T20:22:45Z

Lib/test/support/_hypothesis_stubs/__init__.py

+from . import strategies
+
+
+def given(*_args, **_kwargs):


Why the underscores?

Functions need docstrings. This one should mentions that arguments are not used as randomized strategies are not run.

The underscores are because it's unused. Might be Google-isms or pylint-isms getting to me, because pylint won't complain about unused arguments if you start them with _.

OK, never seen this. It might or might not be an issue for others.

terryjreedy · 2021-05-25T20:22:45Z

Lib/test/support/_hypothesis_stubs/__init__.py

+            # If we have found no examples, we must skip the test. If @example
+            # is applied after @given, it will re-wrap the test to remove the
+            # skip decorator.


Does hypothesis allow intermixing @given and @examples? Should suggestion for stdlib use be that @given is outermost (topmost) decorator?

Hypothesis allows you to do stuff like:

@example(3) @given(i=strategies.integers()) @example(4) def test_i(i): ...

It's easy enough to support that here, so I see no reason to force people to change the order they want to apply their decorators in.

terryjreedy · 2021-05-25T20:22:45Z

Lib/test/support/_hypothesis_stubs/_helpers.py

+# Stub out only the subset of the interface that we actually use in our tests.
+class StubClass:
+    def __init__(self, *args, **kwargs):
+        super().__init__()


I don't believe I have ever seen this in direct object subclass. What does object.__init__() do?

Presumably nothing, but I believe this is relevant when there is some more complicated MRO.

TBH I can never keep the details straight and I can't remember if I did this because it was necessary or out of habit. Looking at it now, it seems like this is the base of a linear hierarchy, so right now super().__init__() will always call object.__init__(), but I'm not sure it's hurting anything...

It raises questions in a reviewers/readers mind, such as "Did you forget to subclass from somthing else?"

terryjreedy · 2021-05-25T20:22:45Z

Lib/test/support/_hypothesis_stubs/__init__.py

+    "Verbosity",
+]
+
+from . import strategies


A linter might recommend removing import as unused. Add comment such # Added to __all__ for users of this module.?

Shouldn't the linter notice that it's part of __all__, or that we're in __init__.py? This is done to expose the strategies name eagerly, so that import hypothesis; hypothesis.strategies.integers will work. it's fairly idiomatic, so I don't expect anyone to be surprised by it.

I was, until I looked up instead of down for its use, and saw usage examples in the tests.

terryjreedy · 2021-05-25T20:22:45Z

Lib/test/test_zoneinfo/test_zoneinfo_property.py

+        # datetimes is always False.
+        if py_dt != c_dt:
+            self.assertEqual(
+                self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt),


Trailing comma is legal but misleading as these are not a series of 'equivalent' args and nothing more can be added.

Suggested change

self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt),

self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt)

Hm.. I've been maintaining the style with black upstream so I think I'd prefer not to change it for trivial reasons, to make merges easier (though this is broken up across multiple lines in the upstream repo, so I dunno what's going on there.

This is not idiomatic in the stdlib I have seen. I understand your concern with your upstream (as in, I won't object further), but this makes me like black even less ;-).

bedevere-bot · 2021-05-25T20:22:48Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

pganssle

2 PRs in 1. Do you consider the hypothesis_helper part to be finished and ready to merge?

The content is probably ready to go, but it's still mostly a draft, so it doesn't have niceties like documentation and NEWS and whatnot. I figured I'd get something that works and allow that to be used as a base for implementing something that works.

I agree this is_sort of_ 2 PRs in one, but we don't have anything that uses hypothesis in the standard library, so I think we need something here in order to exercise the code paths. We could switch it out for a simpler set of hypothesis tests (though presumably a simpler set would exercise a smaller subset of the stubs).

If we break it into multiple PRs, we should do it at the end, immediately before merge.

Lib/test/support/_hypothesis_stubs/__init__.py

pganssle · 2021-05-25T21:09:44Z

Lib/test/support/_hypothesis_stubs/__init__.py

+    "Verbosity",
+]
+
+from . import strategies


Shouldn't the linter notice that it's part of __all__, or that we're in __init__.py? This is done to expose the strategies name eagerly, so that import hypothesis; hypothesis.strategies.integers will work. it's fairly idiomatic, so I don't expect anyone to be surprised by it.

pganssle · 2021-05-25T21:09:44Z

Lib/test/support/_hypothesis_stubs/__init__.py

+from . import strategies
+
+
+def given(*_args, **_kwargs):


The underscores are because it's unused. Might be Google-isms or pylint-isms getting to me, because pylint won't complain about unused arguments if you start them with _.

pganssle · 2021-05-25T21:09:44Z

Lib/test/support/_hypothesis_stubs/__init__.py

+            # If we have found no examples, we must skip the test. If @example
+            # is applied after @given, it will re-wrap the test to remove the
+            # skip decorator.


Hypothesis allows you to do stuff like:

@example(3) @given(i=strategies.integers()) @example(4) def test_i(i): ...

It's easy enough to support that here, so I see no reason to force people to change the order they want to apply their decorators in.

pganssle · 2021-05-25T21:09:44Z

Lib/test/support/_hypothesis_stubs/_helpers.py

+# Stub out only the subset of the interface that we actually use in our tests.
+class StubClass:
+    def __init__(self, *args, **kwargs):
+        super().__init__()


Presumably nothing, but I believe this is relevant when there is some more complicated MRO.

TBH I can never keep the details straight and I can't remember if I did this because it was necessary or out of habit. Looking at it now, it seems like this is the base of a linear hierarchy, so right now super().__init__() will always call object.__init__(), but I'm not sure it's hurting anything...

pganssle · 2021-05-25T21:09:44Z

Lib/test/test_zoneinfo/test_zoneinfo_property.py

+        # datetimes is always False.
+        if py_dt != c_dt:
+            self.assertEqual(
+                self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt),


Hm.. I've been maintaining the style with black upstream so I think I'd prefer not to change it for trivial reasons, to make merges easier (though this is broken up across multiple lines in the upstream repo, so I dunno what's going on there.

Zac-HD · 2021-05-26T02:48:42Z

Lib/test/support/_hypothesis_stubs/__init__.py

+def settings(*args, **kwargs):
+    pass  # pragma: nocover


@hypothesis.settings(...) can be used as a decorator, so perhaps

Suggested change

def settings(*args, **kwargs):

pass # pragma: nocover

def settings(*args, **kwargs):

return lambda f: f # pragma: nocover

Zac-HD · 2021-05-26T02:48:42Z

Lib/test/support/_hypothesis_stubs/__init__.py

+        return list(cls)
+
+
+class Verbosity(Enum):


We might also want the Phase enum?

Zac-HD · 2021-05-26T02:48:42Z

Lib/test/support/_hypothesis_stubs/_helpers.py

+        argstr = ", ".join(self.__stub_args)
+        kwargstr = ", ".join(
+            f"{kw}={val}" for kw, val in self.__stub_kwargs.items()
+        )
+
+        in_parens = argstr
+        if kwargstr:
+            in_parens += ", " + kwargstr
+
+        return f"{self.__qualname__}({in_parens})"


Since the various .map(), .filter(), .__or__() methods don't alter the repr at all, I'd probably prefer that this be less specific - better to represent integers().map(str) as <stub object at 0x...> than integers()!

I also don't think it's worth the code to make the repr track all the methods, unless adding a Stub.__trailing_repr argument would work?

pganssle added the DO-NOT-MERGE label Oct 21, 2020

the-knights-who-say-ni added the CLA signed label Oct 21, 2020

bedevere-bot added the awaiting core review label Oct 21, 2020

Zac-HD approved these changes Oct 22, 2020

View changes

Lib/test/support/_hypothesis_stubs/__init__.py Outdated Show resolved Hide resolved

Lib/test/support/_hypothesis_stubs/__init__.py Show resolved Hide resolved

Zac-HD mentioned this pull request Oct 28, 2020

Add a st.timezones() strategy using the stdlib zoneinfo module HypothesisWorks/hypothesis#2630

Closed

github-actions bot added the stale label Dec 16, 2020

pganssle removed the stale label Dec 16, 2020

pganssle force-pushed the hypothestubs branch from 39da64c to 1ec2c35 Compare Apr 19, 2021

Zac-HD reviewed May 13, 2021

View changes

Lib/test/support/_hypothesis_stubs/__init__.py Outdated Show resolved Hide resolved

Zac-HD mentioned this pull request May 13, 2021

Action items: adopting property-based testing in CPython's CI python/steering-council#65

Open

pganssle added 3 commits May 13, 2021

Add property tests for the zoneinfo module

895602e

This migrates the tests from https://github.com/Zac-HD/stdlib-property-tests into the standard library, using the hypothesis stubs.

Add examples to zoneinfo hypothesis tests

3576fc9

pganssle force-pushed the hypothestubs branch from 1ec2c35 to 3576fc9 Compare May 13, 2021

pganssle added 2 commits May 13, 2021

fixup! Add stubs for hypothesis tests

6b96345

fixup! fixup! Add stubs for hypothesis tests

fad7849

pganssle reviewed May 13, 2021

View changes

terryjreedy requested changes May 25, 2021

View changes

bedevere-bot removed the awaiting core review label May 25, 2021

bedevere-bot added the awaiting changes label May 25, 2021

pganssle reviewed May 25, 2021

View changes

Zac-HD reviewed May 26, 2021

View changes

pganssle mentioned this pull request May 2, 2022

gh-80010: Expand fromisoformat to include most of ISO-8601 #92177

Merged

3 tasks

python / cpython Public

bpo-42109: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests #22863

bpo-42109: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests #22863

pganssle commented Oct 21, 2020 •

edited

Zac-HD left a comment

github-actions bot commented Dec 16, 2020

pganssle May 13, 2021

terryjreedy left a comment

terryjreedy May 25, 2021

pganssle May 25, 2021

terryjreedy May 26, 2021

terryjreedy May 25, 2021

pganssle May 25, 2021

terryjreedy May 26, 2021

terryjreedy May 25, 2021

pganssle May 25, 2021

terryjreedy May 26, 2021

terryjreedy May 25, 2021

pganssle May 25, 2021

terryjreedy May 26, 2021

terryjreedy May 25, 2021

pganssle May 25, 2021

terryjreedy May 26, 2021

bedevere-bot commented May 25, 2021

pganssle left a comment

pganssle May 25, 2021

pganssle May 25, 2021

pganssle May 25, 2021

pganssle May 25, 2021

pganssle May 25, 2021

Zac-HD May 26, 2021

Zac-HD May 26, 2021

Zac-HD May 26, 2021

		raise unittest.SkipTest("No time zone data available")


		def valid_keys():

	self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt),
	self._is_ambiguous(py_dt), self._is_ambiguous(c_dt), (py_dt, c_dt)

python / cpython Public

bpo-42109: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests #22863

Are you sure you want to change the base?

bpo-42109: Implementation of hypothesis stubs for property-based tests, with zoneinfo tests #22863

Conversation

pganssle commented Oct 21, 2020 • edited

Zac-HD left a comment

github-actions bot commented Dec 16, 2020

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented May 25, 2021

pganssle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pganssle commented Oct 21, 2020 •

edited