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.

Sign up for GitHub

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

Jump to bottom

Update documentation and template for reviewers about tests #203

Merged
merged 4 commits into from
Apr 12, 2023
Merged

Update documentation and template for reviewers about tests #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d3d41cb
Specify the check on the tests.
cmarmo Mar 20, 2023
2cbc496
Update reviewer-guide.md
cmarmo Mar 20, 2023
1cc29f5
Update appendices/review-template.md
lwasser Mar 22, 2023
d09b221
'[pre-commit.ci 🤖] Apply code format tools to PR'
pre-commit-ci[bot] Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion appendices/review-template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ Package structure should follow general community best-practices. In general ple
- [ ] **Installation:** Installation succeeds as documented.
- [ ] **Functionality:** Any functional claims of the software been confirmed.
- [ ] **Performance:** Any performance claims of the software been confirmed.
- [ ] **Automated tests:** Tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
- [ ] **Automated tests:**
- [ ] All tests pass on the reviewer's local machine for the package version submitted by the author. Ideally this should be a tagged version making it easy for reviewers to install.
- [ ] Tests cover essential functions of the package and a reasonable range of inputs and conditions.
- [ ] **Continuous Integration:** Has continuous integration setup (We suggest using Github actions but any CI platform is acceptable for review)
- [ ] **Packaging guidelines**: The package conforms to the pyOpenSci [packaging guidelines](https://www.pyopensci.org/python-package-guide).
A few notable highlights to look at:
Expand Down
7 changes: 7 additions & 0 deletions how-to/reviewer-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,13 @@ in the review template, please also provide general comments addressing the foll

If you have your own relevant data/problem that you could use the package to solve, work through it with the package. You may find rough edges and use-cases the author didn't think about.

```{important}
Some maintainers do not ship the tests with their package.
This means that the dependencies for testing may be missing.
Copy link
Member

Choose a reason for hiding this comment

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

@cmarmo i'm curious if even if authors ship tests, would they add a tool like pytest as a dependency for the entire package?

Copy link
Member

Choose a reason for hiding this comment

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

so there would be a dep section in the pyproject.toml file for dependencies but those might not necessarily get installed with the package install ?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly you can define optional dependencies in pyproject.toml eg

[project.optional-dependencies]
test = ["pytest"]
doc = [
  "matplotlib",
  "click",
]

then install with eg

pip install mypackage[test]

Copy link
Member

Choose a reason for hiding this comment

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

thank you! i never knew that you could install that way (ie using [test] but learned about it today!

What do you think about us adding a note about how to install with tests using the example that you provide?

i've also made a note to clarify how to run tests in our package guide as well :)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think about us adding a note about how to install with tests using the example that you provide?

I think it could be helpful for packagers to know about the optional-dependencies metadata.

The keyword in the install command depends on the choice of the maintainer/packager.
In my example the command to install the doc dependencies is

pip install mypackage[doc]

but could have been docs for example.
If the maintainer adds optional-dependencies we might want to check that the install documentation is complete during the review.

Check to have at least pytest installed.
Also, when cloning the repository to run the tests, please check the cloned version aligns with the submitted one.
```

## Submit your review
* When your review is complete, paste it as a comment into the package software-review issue.
* Additional comments are welcome in the same issue. We hope that package reviews will work as an ongoing conversation with the authors as opposed to a single round of reviews typical of academic manuscripts.
Expand Down