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

return_value and not_a_test_method health checks strict error fix #3581

Merged
merged 25 commits into from
Mar 15, 2023

Conversation

reaganjlee
Copy link
Contributor

@reaganjlee reaganjlee commented Feb 13, 2023

This addresses parts of issue #3568.

Currently, some of the tests fail because (I believe) note_deprecation causes an undefined error with Pytest before their expected FailedHealthCheck message in tests that explicitly test for return_value (and not_a_test_method). An example of this is the test_stateful_returnvalue_healthcheck test.

Example below
Screen Shot 2023-02-13 at 1 06 30 PM

Edit: I can solve the majority of tests above by adding in a catch_warning, though I'm not sure whether or not editing the tests themselves would be advised

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Comments below; you'll also need to add your name to AUTHORS.rst and create a hypothesis-python/RELEASE.rst file - see recent PRs for examples. Finally, we'll want to test that accessing the deprecated attributes... emits a HypothesisDeprecationWarning, which you can do with our @checks_deprecated_behaviour decorator.

hypothesis-python/src/hypothesis/_settings.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/_settings.py Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/internal/healthcheck.py Outdated Show resolved Hide resolved
@Zac-HD
Copy link
Member

Zac-HD commented Feb 17, 2023

OK... this is looking like a bit of a tarpit, because Enums turn out to have a lot of metaclass magic inside them already and so our __getattr__ method isn't actually being called, but it's also hard to override the metaclass 🤔

@Zac-HD Zac-HD self-assigned this Feb 17, 2023
@reaganjlee reaganjlee marked this pull request as ready for review February 25, 2023 11:13
@Zac-HD
Copy link
Member

Zac-HD commented Feb 28, 2023

OK, let's give up on deprecating the attribute access - that would have been nicer, but turns out to be way harder than expected.

Instead, we'll note_deprecation() if you pass one of the deprecated enum members to settings(suppress_health_check=...), keep them excluded from the .all() and .__iter__() methods, and trust that this will be enough to warn most users. Back to you @reaganjlee!

@Zac-HD Zac-HD removed their assignment Feb 28, 2023
@reaganjlee
Copy link
Contributor Author

So I got all() and noting deprecation when setting settings working, but I am running into a similar enum issue where iter isn't being called (when calling list(Healthcheck).
Besides that, the other thing I'd like to note is that I removed a test that mainly tested the return_value suppression

@Zac-HD
Copy link
Member

Zac-HD commented Mar 7, 2023

Ugh, maybe we should monkeypatch? something like

class HealthCheck: ...

HealthCheck.__iter__ = lambda: iter(HealthCheck.all())

@reaganjlee
Copy link
Contributor Author

This looks good besides a few (unrelated?) tests. Let me know if there's any issue with adding in a metaclass to do this as well

@Zac-HD Zac-HD requested a review from DRMacIver as a code owner March 12, 2023 17:33
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Looks great! I think we're done here, with the only remaining blocker that infuriating test hang 😢

Comment on lines 464 to 468
@classmethod
def all(cls) -> List["HealthCheck"]:
# Skipping of deprecated attributes is handled in HealthCheckMeta.__iter__
# TODO: note_deprecation() and write a codemod for HC.all() -> list(HC)
return list(HealthCheck)
Copy link
Member

Choose a reason for hiding this comment

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

Want to take this on as a follow-up PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I can work on this after the cacheing :)

hypothesis-python/RELEASE.rst Outdated Show resolved Hide resolved
hypothesis-python/src/hypothesis/_settings.py Outdated Show resolved Hide resolved
hypothesis-python/tests/cover/test_settings.py Outdated Show resolved Hide resolved
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.

None yet

3 participants