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

Update setup and testing docs dev guide #14024

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

cdeil
Copy link
Contributor

@cdeil cdeil commented Aug 11, 2024

Follow-up to #13992 to remove the pytest tests/integration instruction in the dev docs which doesn't exist any more.

@cdeil
Copy link
Contributor Author

cdeil commented Aug 11, 2024

The dev guide is very nice - thank you for writing that up!
https://docs.bokeh.org/en/latest/docs/dev_guide.html

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 export BOKEH_RESOURCES=absolute-dev but then run into this test collection RuntimeError from tests/unit/bokeh/test_resources.py.

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?

(bkdev) bokeh $ pytest tests/unit         
=============================================================================================== test session starts ================================================================================================
platform darwin -- Python 3.12.5, pytest-7.4.4, pluggy-1.5.0
rootdir: /Users/cdeil/code/oss/bokeh
configfile: pyproject.toml
plugins: cov-5.0.0, html-4.1.1, metadata-3.1.1, asyncio-0.21.1, anyio-4.4.0, timeout-2.3.1, xdist-3.6.1
asyncio: mode=Mode.STRICT
timeout: 90.0s
timeout method: signal
timeout func_only: False
collected 11255 items / 1 error                                                                                                                                                                                    

Bokeh selenium tests using 'chrome' driver (no-console-error assertions: ON)

====================================================================================================== ERRORS ======================================================================================================
_______________________________________________________________________________ ERROR collecting tests/unit/bokeh/test_resources.py ________________________________________________________________________________
tests/unit/bokeh/test_resources.py:45: in <module>
    raise RuntimeError("Cannot run the unit tests with BOKEH_RESOURCES set")
E   RuntimeError: Cannot run the unit tests with BOKEH_RESOURCES set
============================================================================================= short test summary info ==============================================================================================
ERROR tests/unit/bokeh/test_resources.py - RuntimeError: Cannot run the unit tests with BOKEH_RESOURCES set
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
================================================================================================= 1 error in 3.85s =================================================================================================

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-3.6@5aecc61). Learn more about missing BASE report.

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           

@bryevdv
Copy link
Member

bryevdv commented Aug 11, 2024

The instructions how to set BOKEH_RESOURCES many options are provided without guidance which one to pick to be able to run the tests.

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. BOKEH_RESOURCES Is mostly useful when running examples. I suppose that could be stated explicitly somewhere in the docs, although we did try to make it clear with the error message "Cannot run the unit tests with BOKEH_RESOURCES set".

@cdeil
Copy link
Contributor Author

cdeil commented Aug 11, 2024

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?

@bryevdv
Copy link
Member

bryevdv commented Aug 11, 2024

I can add a comment making it explicit to unset BOKEH_RESOURCES when working on the tests.

👍 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)

For docs it's the same, no BOKEH_RESOURCES or other env vars should be set?

That's too strong of a statement, usually you will want to set BOKEH_DOCS_CDN=local as described in the docs:

https://docs.bokeh.org/en/latest/docs/dev_guide/documentation.html#build-bokeh-s-documentation

as well as some value (even junk) for GOOGLE_API_KEY also described in the docs:

https://docs.bokeh.org/en/latest/docs/dev_guide/documentation.html#set-environment-variable

(it doesn't have to be set via conda env config and that's not how I do it, though)

@bryevdv
Copy link
Member

bryevdv commented Aug 11, 2024

cc @tcmetzger for thoughts

@cdeil
Copy link
Contributor Author

cdeil commented Aug 11, 2024

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)

Done in 92a723b

GOOGLE_API_KEY
(it doesn't have to be set via conda env config and that's not how I do it, though)

Haven't seen any other project that requires devs to set such an env variable for Sphinx build locally.
Would changing Sphinx config to assume the default dummy value and removing this step for contributors be OK for you?

@bryevdv
Copy link
Member

bryevdv commented Aug 12, 2024

Haven't seen any other project that requires devs to set such an env variable for Sphinx build locally.
Would changing Sphinx config to assume the default dummy value and removing this step for contributors be OK for you?

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".

@bryevdv
Copy link
Member

bryevdv commented Aug 12, 2024

LGTM but I'd like to give @tcmetzger a chance to look over as well.

Copy link
Member

@tcmetzger tcmetzger left a 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!

@tcmetzger tcmetzger merged commit 3ed9201 into bokeh:branch-3.6 Aug 13, 2024
24 checks passed
Chiemezuo pushed a commit to Chiemezuo/bokeh that referenced this pull request Aug 27, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants