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: code coverage & CI pages to guide #104

Closed
wants to merge 10 commits into from
Closed

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Sep 26, 2023

This is a draft for community developed text that i am now turning into a PR for the tests section.


Copy link
Contributor

@sneakers-the-rat sneakers-the-rat left a comment

Choose a reason for hiding this comment

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

thanks for your work on this leah :):):):) this is SUPER IMPORTANT and i have been needing something like this for so long.

I think you cover the important points well here, my comments are mostly nitpicky things that could be improved, but take or leave all of them!

One general comment i would have is that tests are also related to the structure of code, so a short section on "writing testable code" and how tests can force you to write better code just by virtue of how messy code is really difficult to write tests for might be useful? - so an example would be a bigass MATLAB style scientific analysis function that is like 1000 lines long and it loads/saves a bunch of files, plots inline, etc. I have a (super brief, only had a few hours to put together this thing) example here

I also think that a very minimal working example repo would be lovely to have here - eg. if the section could take the example you have with the temperature sensor and keep it running throughout - we write a few functions from different parts of the "program" and then write the corresponding tests so we can have examples for everything, and then when we get to the actually running your tests part we can just literally run the tests for our code, demonstrate what it looks like when a test fails, how to fix it, etc. with screenshots of pytest, nox, what it looks like in a pull request when you have CI enabled, etc. (I'd be happy to help write those and also any of the other recommendations i make here!!!! not trying to just demand labor from you and not volunteer!!!!)

ci-tests-data/index.md Outdated Show resolved Hide resolved
ci-tests-data/index.md Outdated Show resolved Hide resolved
Test types: unit, integration, functional <test-types>
Run tests in CI & locally <run-tests>
Package data <data>
CI <ci>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a separate page, or in run-tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

i can't decide actually and would love feedback. this is why i kind of kept that landing page blank.

I feel like for beginners we want to have a CI page somewhere that introduces it as a concept and what it can do for them.

but then a user may want to set it up for both doc builds, tests and the actual package build. so it could go into 3 different parts of this guide. so MAYBE we just have the CI for tests in this section? and the general CI page lives... somewhere else in the guide? i'm not sure and welcome feedback.


Writing sets of test functions and methods, also known as test suites, for your
package is important for both you as a maintainer, your users, and any potential
new contributors. Test suites consist of sets of functions, methods, and classes
Copy link
Contributor

Choose a reason for hiding this comment

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

sort of redundant. maybe "Each function, method, and class makes sure a ..."


## Why create tests for your package

Tests act as a safety net for code changes. They help you spot and rectify bugs
Copy link
Contributor

Choose a reason for hiding this comment

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

Bullet points?

  • Avoid breaking things - tests make sure that new features or bugfixes don't break things in the package. If you don't have a formal test suite, you probably already run tests, just manually! Tests automate all the manual checks you might do when making changes to your code and allow you to make changes with confidence
  • Collaboration - tests make it possible for people outside your team to contribute to the project without needing an exhaustive code review, and without them needing to understand every detail of the project. If you have tests, and the tests pass with the new code, you've avoided all the labor that usually goes into reviewing and contributing.
  • Edge cases - ...

ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/data.md Outdated Show resolved Hide resolved
@@ -0,0 +1,16 @@
# What is continuous integration?
Copy link
Member Author

Choose a reason for hiding this comment

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

Where this page lives in the guide is still not known... it will be fleshed out to contain an overview of what CI is, why it's useful and various platform options focusing on github actions examples that people can use out of the box.

@lwasser lwasser marked this pull request as ready for review October 2, 2023 19:31
ci-tests-data/tests-ci.md Outdated Show resolved Hide resolved
Comment on lines 65 to 68
run: |
echo "PY=$(python -c 'import hashlib, sys;print(hashlib.sha256(sys.version.encode()+sys.executable.encode()).hexdigest())')" >> $GITHUB_OUTPUT
echo "PIP_CACHE=$(pip cache dir)" >> $GITHUB_OUTPUT
Copy link
Contributor

Choose a reason for hiding this comment

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

Being able to cache things (beyond the cache: pip thing I mentioned above) is a great thing to include! I have a feeling a lot of people will want to customize caching in some way. Would it help to add a comment describing what these lines do?

Copy link
Member Author

Choose a reason for hiding this comment

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

@namurphy it would be super helpful. are you able to suggest some language? i absolutely understand caching and the value of caching but i don't understand why

  1. above we use pip as the cache value - why?
  2. what does this section generally do?

many thanks!! caching is a great addition to this and we can mention it too in our main Ci pages / tutorials.

ci-tests-data/tests-ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/code-cov.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Zeitsperre Zeitsperre left a comment

Choose a reason for hiding this comment

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

Just a quick overview, looking great!

ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/code-cov.md Outdated Show resolved Hide resolved
ci-tests-data/code-cov.md Outdated Show resolved Hide resolved
ci-tests-data/index.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/run-tests.md Outdated Show resolved Hide resolved
ci-tests-data/tests-ci.md Outdated Show resolved Hide resolved
ci-tests-data/tests-ci.md Outdated Show resolved Hide resolved
@lwasser
Copy link
Member Author

lwasser commented Dec 15, 2023

@willingc this is the PR that i'd love to tackle next week. it's been open since september. i will clean up all of the conflicts and prioritize it for tuesday when i'm back online! i'd love to have your eyes on it as well if you can help review! ✨

@lwasser
Copy link
Member Author

lwasser commented Dec 16, 2023

ok y'all. i am going to focus on cleaning up this PR for the next week. a few things you'll see me doing

  1. i'm going to try to address all of the existing comments in this to the best of my ability and
  2. then will pull all of the changes into new pr's within only single files. this will allow us to review / merge things more quickly without having to sort through 16 files!

ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
ci-tests-data/ci.md Outdated Show resolved Hide resolved
lwasser and others added 5 commits January 16, 2024 11:39
ENH: fixes from Jonny's review

Fix: review edits from Jonny p2

Fix: typos and cleanup

Fix: add example to tests ci page
Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/code-cov.md

Co-authored-by: Nick Murphy <[email protected]>

Fix: edits from @namurphy to run tests page

Co-authored-by: Nick Murphy <[email protected]>

Update ci-tests-data/run-tests.md

Co-authored-by: Nick Murphy <[email protected]>
Co-authored-by: Trevor James Smith <[email protected]>

Fix: edit from trevor

Co-authored-by: Trevor James Smith <[email protected]>

Fix: edit from trevor

Co-authored-by: Trevor James Smith <[email protected]>

Fix: edits from Trevor

Co-authored-by: Trevor James Smith <[email protected]>

Fix: edits from Nick

Co-authored-by: Nick Murphy <[email protected]>

Packaging image

Fix: remove unused block

Fix

Update ci-tests-data/tests-ci.md

Co-authored-by: Trevor James Smith <[email protected]>
Co-authored-by: Carol Willing <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Carol Willing <[email protected]>

Update ci-tests-data/ci.md

Co-authored-by: Carol Willing <[email protected]>

Fix: other edits from review'
@lwasser lwasser changed the title Add: tests section to guide Add: code coverage & CI pages to guide Jan 16, 2024
@lwasser lwasser closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-content New feature or request updates-underway
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants