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

Enable print_blob for CIs, disable for local #2512

Merged
merged 5 commits into from
Jul 26, 2020

Conversation

hugovk
Copy link
Contributor

@hugovk hugovk commented Jul 25, 2020

Fixes #2451.

For #2510.

@Zac-HD
Copy link
Member

Zac-HD commented Jul 25, 2020

Hey @hugovk! Please:

  • add yourself to the alphabetical list of authors in CONTRIBUTING.rst
  • add a RELEASE.rst file - check the other PRs for examples.
  • add show_default=False for the setting, and explain in the docstring that the default is True in CI or False in local development

(and thanks for the PR, of course 😍)

@hugovk
Copy link
Contributor Author

hugovk commented Jul 25, 2020

Updated!

And btw I checked for an undocumented CI env var in Azure Pipelines, but not found. Maybe they'll add it one day: GitHub Actions only recently added CI (python-pillow/Pillow#4562).

AP env var docs: https://docs.microsoft.com/en-us/azure/devops/pipelines/build/variables?view=azure-devops&tabs=yaml

@hugovk hugovk force-pushed the smarter-print_blob-default branch from 2d602ab to 53835be Compare July 25, 2020 16:36
@hugovk
Copy link
Contributor Author

hugovk commented Jul 25, 2020

Ha, I must have hit a special small window: I added RELEASE.rst when there was one in master, then fixed the conflict, but I guess the release bot deletes that so got a conflict again!

Rebased!

@Zac-HD
Copy link
Member

Zac-HD commented Jul 25, 2020

  • rebase on master (so you're creating a minor release file)
  • specify in the docstring exactly how the detection works ("True if the X or Y env vars are set, False otherwise", and use Sphinx's envvar role for bonus points)
  • look at the failing tests and change the assertions to account for is_in_ci().

And then I think we'll be done!

@hugovk
Copy link
Contributor Author

hugovk commented Jul 25, 2020

  • specify in the docstring exactly how the detection works ("True if the X or Y env vars are set, False otherwise", and use Sphinx's envvar role for bonus points)

By docstring, do you mean in description= of settings._define_setting, or to add a proper """...""" one to is_in_ci()?

@Zac-HD
Copy link
Member

Zac-HD commented Jul 25, 2020

The value of the description argument, which is eventually formatted into a docstring and rendered in our public docs. Sorry for any confusion!

@hugovk
Copy link
Contributor Author

hugovk commented Jul 25, 2020

Okay! How's that?

  • look at the failing tests and change the assertions to account for is_in_ci().

Hmm, do we want those tests to call is_in_ci() to decide what to expect? If so, should we move is_in_ci() to some utility file?

@Zac-HD
Copy link
Member

Zac-HD commented Jul 25, 2020

Yes, but let's leave it in _settings.py near its only use in the non-test code.

@Zac-HD
Copy link
Member

Zac-HD commented Jul 26, 2020

Hmm. Looking at the actual change, I think it's actually going to be cleaner if we just add print_blob=False to the settings decorator on those inner test functions (or add the decorator too, in one case). Sorry I didn't pick this up before - I think we're otherwise ready to merge!

@hugovk hugovk force-pushed the smarter-print_blob-default branch from ab789a2 to db962aa Compare July 26, 2020 09:42
@hugovk hugovk force-pushed the smarter-print_blob-default branch from db962aa to 3cb3ab4 Compare July 26, 2020 09:54
@hugovk
Copy link
Contributor Author

hugovk commented Jul 26, 2020

The docs build complains about:

/home/vsts/work/1/s/hypothesis-python/docs/../src/hypothesis/__init__.py:docstring of hypothesis.settings.print_blob:1: WARNING: 'envvar' reference target not found: CI
/home/vsts/work/1/s/hypothesis-python/docs/../src/hypothesis/__init__.py:docstring of hypothesis.settings.print_blob:1: WARNING: 'envvar' reference target not found: TF_BUILD

https://dev.azure.com/HypothesisWorks/Hypothesis/_build/results?buildId=4110&view=logs&j=6fe85d43-0322-5a7e-7a5c-a08b11a8fdaa&t=7b5ce327-7af2-52df-14b0-47f2945e41c0

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.

🎉

@Zac-HD Zac-HD merged commit a5c01a0 into HypothesisWorks:master Jul 26, 2020
@hugovk hugovk deleted the smarter-print_blob-default branch July 26, 2020 16:37
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.

Smarter default value for the print_blob setting
2 participants