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. |
tcmetzger
left a comment
There was a problem hiding this comment.
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
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Follow-up to #13992 to remove the
pytest tests/integrationinstruction in the dev docs which doesn't exist any more.