-
Notifications
You must be signed in to change notification settings - Fork 5
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 readme with Usage instructions #39
Conversation
Ping @GeorgianaElena The ReadTheDocs build is failing and I think it is because of the
Do you think we should pin the |
Since RTD is a "production" deployment of our docs we could pin all deps and let dependabot bump them? If you want we could also switch to |
@manics Thanks for the suggestion. I have looked over the
|
Ping @GeorgianaElena could you please help review this PR? |
README.md
Outdated
[![Latest PyPI version](https://img.shields.io/pypi/v/pytest-jupyterhub?logo=pypi)](https://pypi.python.org/pypi/pytest-jupyterhub) | ||
[![Documentation status](https://img.shields.io/readthedocs/pytest-jupyterhub?logo=read-the-docs)](https://pytest-jupyterhub.readthedocs.io/en/latest/?badge=latest) | ||
[![GitHub Workflow Status - Test](https://img.shields.io/github/workflow/status/jupyterhub/pytest-jupyterhub/Test?logo=github&label=tests)](https://github.com/jupyterhub/pytest-jupyterhub/actions) | ||
[![Test coverage of code](https://codecov.io/gh/jupyterhub/pytest-jupyterhub/branch/main/graph/badge.svg)](https://codecov.io/gh/jupyterhub/pytest-jupyterhub) |
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.
Is https://codecov.io/gh/jupyterhub/pytest-jupyterhub meant to be public?
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.
I am not sure I understand what you mean by 'public', but we have not set up the project on codecov yet. I don't think I have the relevant permissions for that. I can delete this badge for now until that is done.
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.
codecov
doesn't look to be setup for this repo yet. I believe it makes sense to not have a badge for it.
Also, @Sheila-nk, can you please open up an issue about enabling codecov for this repo?
README.md
Outdated
@@ -17,8 +29,63 @@ For more information on Fixtures, check out this [pytest documentation](https:// | |||
A **Mock** is an object that simulates the behavior of another object such as a class or function. They are used to simulate the behavior of real objects for testing purposes. | |||
For more information on Mocks, check out this [unittest documentation](https://docs.python.org/3/library/unittest.mock.html) on the mock module. | |||
|
|||
**Example:** | |||
### Example |
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.
Is it worth deleting this "Example" section? It's a bit confusing having two "Example" sections in the README, and this one goes into a lot of technical detail. The usage example you've added below seems much more relevant and helpful to readers.
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.
Okay, I agree. I will remove this example section.
README.md
Outdated
|
||
# [Reusable JupyterHub Pytest Plugin](https://github.com/jupyterhub/pytest-jupyterhub) | ||
|
||
[![Latest PyPI version](https://img.shields.io/pypi/v/pytest-jupyterhub?logo=pypi)](https://pypi.python.org/pypi/pytest-jupyterhub) |
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.
If we're adding a pypi badge with link we should push something there to claim ownership, even if it's a placeholder package.
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.
@GeorgianaElena suggested we make an alpha release of this project. We can hold on adding the pypi badge until that is done.
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.
@Sheila-nk, can you please open an issue about making a release and adding the corresponding docs, GitHub workflow, badge, etc, to make sure we keep track of it? Thank you ❤️
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.
Here it is: #45
You can add any steps I have missed or remove any unnecessary ones.
README.md
Outdated
|
||
Each of these hub components and the hub itself define their own testing infrastructure, building everything from the ground up using the **pytest** framework. And some of this complex work is either repetitive across JupyterHub sub-projects, or under-specified for some of them. This has sparked a need to abstract these common parts into a separate testing framework. | ||
Each of these hub components and the hub itself define their own testing infrastructure, building everything from the ground up using the [**pytest**](https://docs.pytest.org/en/7.2.x/) framework. And some of this complex work is either repetitive across JupyterHub sub-projects, or under-specified for some of them. This has sparked a need to abstract these common parts into a separate testing framework. |
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.
Let's link to the pytest stable or latest docs, instead of a certain version, as it might not always be the version used by this project.
README.md
Outdated
|
||
To use the **JupyterHub Pytest Plugin**, you will first need to install it using pip by either: | ||
|
||
- installing it locally |
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.
I believe it's worth leaving a note here about the fact that we're instructing to install the package from GitHub because the package was not yet released on PyPI. And then change the install instruction once that's done. Also, adding this to a todo list, somewhere in an issue to not forget, will be also a good idea.
README.md
Outdated
|
||
- installing it locally | ||
```bash | ||
pip install --upgrade pip |
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.
Let's remove this step, as upgrading pip is not a requirement of the project.
README.md
Outdated
- adding it to the `requirements.txt` file of your project: | ||
|
||
```python | ||
# in requirements.txt | ||
git+https://github.com/jupyterhub/pytest-jupyterhub.git@main | ||
``` |
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 is subject to how each project is managing their dependencies, and it's hard and not recommended to cover all cases, so let's remove and leave it to each project to define.
README.md
Outdated
```bash | ||
pip install --upgrade pip | ||
pip install -r requirements.txt | ||
``` |
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.
When installing a package with pip install git+https://github.com/jupyterhub/pytest-jupyterhub.git@main
, its dependencies get installed also.
Plus, we're defining the pytest-jupyterhub
's deps in pyproject.toml
and don't actually have a requirements.txt
file in this repo, right?
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.
Oh yes! That is my mistake. All dependencies are defined in pyproject.toml
. No requirements.txt
file for this project, except for the docs.
README.md
Outdated
## Usage | ||
|
||
To use a specific fixture or mock, import it from its module in the `pytest_jupyterhub` package: | ||
|
||
```python | ||
from pytest_jupyterhub.jupyterhub_spawners import hub_app | ||
``` |
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.
The pytest docs describe differently how to use a plugin in a test suite: https://docs.pytest.org/en/7.1.x/how-to/writing_plugins.html#requiring-loading-plugins-in-a-test-module-or-conftest-file
Is there a reason why we're taking a different approach here and importing the fixtures same like we'd do from a regular Python package?
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.
Hmm, true. I might need to change the DockerSpawner
draft PR as well since I imported it instead of loading the plugin. Thank you for catching this.
for more information, see https://pre-commit.ci
Co-authored-by: Simon Li <[email protected]>
3f66035
to
9f8fe41
Compare
@GeorgianaElena I have worked on the recommended changes and added a |
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 great to me! I made two code suggestions that I think makes sense.
@Sheila-nk btw amazing README.md file, I love it! It does a perfect job introducing a reader I think - it doesn't assume too much knowledge ahead of time and onboards the reader, and it doesn't try to explain too much either. Perfectly balanced in my mind! |
Co-authored-by: Erik Sundell <[email protected]>
Co-authored-by: Erik Sundell <[email protected]>
Thank you @consideRatio for your kind words and review ❤️ |
DockerSpawner
draft PRCloses #37