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

Add bootstrap pip spec to the integration test docs #558

Merged
merged 1 commit into from Apr 30, 2020

Conversation

jtpio
Copy link
Contributor

@jtpio jtpio commented Apr 27, 2020

Update the docs to run the integration tests locally.

The bootstrap_pip_spec parameter can be specified before the test files to avoid shifting the arguments, similar to the Circle CI script:

.circleci/integration-test.py run-test basic-tests \
"$BOOTSTRAP_PIP_SPEC" test_hub.py test_install.py test_extensions.py \
<< parameters.upgrade >>

image

  • Add / update documentation
  • Add tests

@jtpio
Copy link
Contributor Author

jtpio commented Apr 27, 2020

As an alternative, we might also want to specify the full bootstrap pip spec instead of "".

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch @jtpio. I didn't realize the instructions were no longer up to date after updating the tests 😕

Since the bootstrap_pip_spec arg was added for the upgrade tests, I don't think is of much relevance when running the integration tests. Now thinking, if I'd have made the bootstrap_pip_spec argument optional, we wouldn't have needed to force people add an empty string in the list of args (which can easily be forgotten) or the entire pip spec of TLJH.

What do you think about making the bootstrap_pip_spec so that if people want to use it, they would do so with:

--bootrap_pip_spec <some-pip-spec>

And if they just want the default one, they just run the tests as usual, no empty string needed.

Update: Opened #563 with this change

@jtpio
Copy link
Contributor Author

jtpio commented Apr 30, 2020

And if they just want the default one, they just run the tests as usual, no empty string needed.

That would be really nice. An empty string is not very explicit and after reading the updated docs again (from this change), it felt a bit odd. An optional parameter sounds good 👍

Update: Opened #563 with this change

Great, thanks!

I can update this PR to the mention the optional bootstrap_pip_spec in the docs. Or we can also close it and do everything in #563, that also works.

@GeorgianaElena
Copy link
Member

@jtpio, I've just merged #563.
I'd be great if you could update this PR 🚀

@jtpio
Copy link
Contributor Author

jtpio commented Apr 30, 2020

Perfect!

Just updated the PR and rebased on the latest master.

Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks @jtpio 🌞

@GeorgianaElena GeorgianaElena merged commit 8841bf0 into jupyterhub:master Apr 30, 2020
@jtpio jtpio deleted the integration-tests-docs branch April 30, 2020 15:12
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.

None yet

2 participants