✏️ Fill in missing steps in CONTRIBUTING.md #1276
Conversation
dannysepler
requested review from
alexcjohnson and
Marc-Andre-Rivet
as code owners
dannysepler
changed the title
| @@ -12,7 +12,7 @@ | |||
| "private::lint.renderer": "cd dash-renderer && npm run lint", | |||
| "private::test.setup-nested": "cd \\@plotly/dash-generator-test-component-nested && npm ci && npm run build && pip install -e .", | |||
| "private::test.setup-standard": "cd \\@plotly/dash-generator-test-component-standard && npm ci && npm run build && pip install -e .", | |||
| "private::test.unit-dash": "PYTHONPATH=~/dash/tests/assets pytest tests/unit", | |||
There was a problem hiding this comment.
was breaking for me since i didn't clone dash into ~/
There was a problem hiding this comment.
heh nor did I - looks like this was originally hard-coded in our circleci setup, then moved out of there when we created a dash-level package.json in #1151 but not generalized. TBH locally I've always run the desired tests directly via pytest but this is certainly better 🎉
Actually it would be cleaner to do something inside unit/test_app_runners.py rather than rely on PYTHONPATH on the command line. It'd be nice if you could just run pytest and have it work. Apparently that was never the case for this test... I wonder if we could just chdir into tests/assets? sys.path won't work because the app gets started in a new process.
Anyway @dannysepler not something you need to worry about unless you're motivated to try. I'm just thinking out loud.
There was a problem hiding this comment.
totally agreed! big fan of just running pytest -- it's a lot more intuitive for new peeps like me. unfortunately i still can't get the tests working lol (attached an issue about it), so i'm scared to do anything beyond the super surface-level
CONTRIBUTING.md
Outdated
| @@ -116,6 +119,8 @@ Note: *You might find out that we have more integration tests than unit tests in | |||
|
|
|||
| We introduced the `dash.testing` feature in [Dash 1.0](https://community.plotly.com/t/announcing-dash-testing/24868). It makes writing a Dash integration test much easier. Please read the [tutorial](http://dash.plotly.com/testing) and add relevant integration tests with any new features or bug fixes. | |||
|
|
|||
| To run the integration tests, you may have to [install circleci](https://circleci.com/docs/2.0/local-cli/). | |||
There was a problem hiding this comment.
Oh - I see, because the npm script private::test.integration-dash uses the circleci commands for splitting tests across multiple containers... @Marc-Andre-Rivet I see why you did it this way but in retrospect baking circleci into package.json seems a bit unfortunate... maybe we should mark the top-level scripts as meant for CI (so test.unit->citest.unit etc?), and make a new script test just for running locally (more for documentation purposes than actually to be run, but it'd be nice if it DID run!). Let me see if I can fix the unit/test_app_runners.py issue discussed previously, then I think this would be something very simple like:
"test": "pytest && cd dash-renderer && npm run test"
There was a problem hiding this comment.
Fixed test_app_runners.py in dc837bd and c865257, and updated the scripts in a481b94 - @Marc-Andre-Rivet are you OK with this?
Then I took out this now-obsolete note about the circleci CLI and made a few more updates to CONTRIBUTING in bce1a37
FWIW if I run the entire pytest suite locally I currently get three errors, repeatably: cbcx001, rddd001, and rdif001. Haven't looked into it, and all three of these pass on CI.
There was a problem hiding this comment.
wow, thanks for all your help here alex! a further point to why i'm happy you made a new target, the circleci tests glob method requires "circleci-agent" available as a local bash command which was... hard to find documentation for? (i'm also a circleci newbie, so perhaps installation is very simple.) this is all to say -- this is MUCH better now for new devs!
as far as the three tests: i also have 3 failing tests but they're given by test name for me? test_browser_smoke, test_process_server_smoke, and test_threaded_server_smoke fail on <<Webdriver not initialized correctly>> -- is this what you're referring to as well?
There was a problem hiding this comment.
hmm no, cbcx001 for example is test_cbcx001_modified_response, from tests/integration/callbacks/test_callback_context.py
We should really extend the test-case-ID pattern to the unit tests as well - I think they were left out of that pattern mainly because the unit tests run so much faster than integration tests, but it'd still be nice to be able to easily target unit tests, and just for uniformity.
If those ones are failing, sounds like a problem with chromedriver or something? But if the integration tests pass then things are generally working fine, so I'm not sure what the problem could be.
There was a problem hiding this comment.
Gotcha, sounds like a me problem then :) thanks for explaining!
Looks like the CONTRIBUTING.md file was missing a few steps with npm?
NB: the black pre-commit failed on three files (
test_hot_reload.py,testing/plugin.py, andapplication_runners.py). As a result, I committed with--no-verify. These pre-commit errors should be resolved here: #1277Contributor Checklist
This checklist feels N/A cause there's no code change.