-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update setup and testing docs dev guide #14024
Conversation
The dev guide is very nice - thank you for writing that up! The instructions how to set BOKEH_RESOURCES many options are provided without guidance which one to pick to be able to run the tests. I chose the first When I try other options I get many unit tests to run, but still dozens of fails (and it's very slow). Can you please improve the description here how to set it?
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## branch-3.6 #14024 +/- ##
=============================================
Coverage ? 93.10%
=============================================
Files ? 282
Lines ? 19850
Branches ? 0
=============================================
Hits ? 18481
Misses ? 1369
Partials ? 0 |
There are no instructions for tests because no value should be set at all for the tests. Tests always manage their own resources in the appropriate and necessary way. |
I was assuming that if I follow "setup", then "tests", then "docs" all would just work and I could use one terminal for any task. I can add a comment making it explicit to unset BOKEH_RESOURCES when working on the tests. For docs it's the same, no BOKEH_RESOURCES or other env vars should be set? |
👍 Seems fine, it's definitely always my assumption that env vars should not be set unless explicitly mentioned, but perhaps that is assuming too much on my part. Edit: Or maybe better, to just update the BOKEH_RESOURCES docs subsection to make it clear when they are appropriate (i.e. really only when running examples, and wanting to use a local version of BokehJS instead of the latest CDN version)
That's too strong of a statement, usually you will want to set https://docs.bokeh.org/en/latest/docs/dev_guide/documentation.html#build-bokeh-s-documentation as well as some value (even junk) for https://docs.bokeh.org/en/latest/docs/dev_guide/documentation.html#set-environment-variable (it doesn't have to be set via |
cc @tcmetzger for thoughts |
Done in 92a723b
Haven't seen any other project that requires devs to set such an env variable for Sphinx build locally. |
No, not really. Most doc deployments are automated in CI but occasionally it is necessary to make manual updates. Having a default dummy value risks seeing a bad deployment getting accidentally published live. Explicitly requiring a key to be provided is "fail-safe". |
LGTM but I'd like to give @tcmetzger a chance to look over as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good thank you!
I'd say let's get this merged - I do think that we should improve that whole "Set environment variables" section. Let's make a follow-up issue for that!
* Update setup and testing docs dev guide * Update integration test note in testing.rst * Remove mention of non-existing .coveragerc in testing.rst * Add note about BOKEH_RESOURCES in dev guide
Follow-up to #13992 to remove the
pytest tests/integration
instruction in the dev docs which doesn't exist any more.