Enh: Add tests pages - run tests in ci and locally to guide #145

lwasser · 2024-01-03T22:37:16Z

This is ROUND 2 of the community review of the tests pr that we started back in september

see pr #104 for reference.

This has the next two pages in that section in a edited format from the first review

run tests locally (using nox)
run tests online (using ci / github actions)

Reviews are welcome on this pr! Given this is a meatier PR, let's try to get reviews in by 15 January 2024 with the goal of merging the following week. i'll try to chip away at feedback as it comes in to stay on top of things!

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206246014382509

tests/run-tests.md

tests/tests-ci.md

ucodery · 2024-01-04T18:32:25Z

tests/tests-ci.md

+      - name: Install dependencies
+        run: |
+          python -m pip install --upgrade pip
+          python -m pip install nox


With a cache, this install should probably also be an --upgrade

ok that makes a lot of sense. @ucodery i asked this question in slack since it came up in the review as well. the caching part of this workflow is really complex. i think for a beginner it will be overwhelming. i'm curious what people think about how much efficiency caching really adds especially for someone new to packaging. what are your thoughts on that?

even if it adds considerable benefit it feels like cognitive overload to me in a beginner friendly tutorial but might be good in a more advanced tutorial (ie series part 2?)

ok we have collectively decided to remove the caching part in this ppage just to keep things simple.

i opened an issue to capture this here and we can revisit it once the core pieces of the guidebook are all pulled together.

Midnighter

Added some smaller comments.

Was there a larger discussion on discourse around promoting nox? I didn't see anything on Slack. It's, of course, a fine choice and I'm very late to the party, but I'm personally against nox for the simple reason that I need to write Python code that I may introduce bugs in rather than simply configuring the tool as I would for tox, for example.

tests/run-tests.md

Midnighter · 2024-01-08T12:18:59Z

tests/tests-ci.md

+    steps:
+      - uses: actions/checkout@v4
+        with:
+          # fetch more than the last single commit to help scm generate proper version


It is never mentioned what scm is.

Also 20 seems like a magic number to me. I personally never had an issue as long as the depth is >1.

it is totally random and magic and will vary based upon people's commit and release cadence.

but here is what i'm thinking about now based on your comments here and below.

this action is REALLY COMPLEX with the cache and fetch depth pieces. AND we will cover fetch depth when we cover SCM based release / versioning - not here. so what if we remove both caching (and add it as an admonition perhaps??) And the SCM fetch depth stuff so we can focus ONLY on running tests here. that is what my gut is telling me would be most beginner friendly. so i'll go ahead and do it and we can always add. the other stuff back OR have an example in our package repo with the more complex example of the action that supports scm (and maybe caching??)

I completely agree. I think, this guide would benefit a lot from ignoring all the caching part and just focusing on running the tests part.

Midnighter · 2024-01-08T12:22:20Z

tests/tests-ci.md

+        uses: actions/setup-python@v4
+        with:
+          python-version: ${{ matrix.python-version }}
+          cache: pip # By adding cache here, you are telling actions to reuse installed dependencies rather than re-downloading and installing them each time. This speeds up your workflow


I'm somewhat doubtful about caching. All files that you cache still have to be transferred from GH's storage to the worker machine. That is faster than fetching from a random internet source, but still is a network request. Secondly, I actually like to not cache because then I get the latest versions installed with every run. That way I can discover new versions of dependencies that somehow break my package faster.

ok - from my perspective the issue of caching on this page is beyond the scope of this page. so i wrote this above - i'm leaning towards removing it altogether just to keep the example simple. we could have another page on more complex test builds too!

@namurphy does that seem reasonable to you for this page? to just start simpler because most of our users will be on the newer end of packaging?

we could then have a page devoted to more efficient CI workflows or add sections to these pages with those more complex concepts covered. The code in this action is a bit tricky to follow and explain. And i don't know enough in terms of how much efficiency it does or does not provide. i'll post in slack as well in case folks want to discuss.

Midnighter · 2024-01-08T12:24:31Z

tests/tests-ci.md

+      # You only need to upload code coverage once to codecov
+      - name: Upload coverage to Codecov
+        if: ${{ matrix.os == 'ubuntu-latest' &&  matrix.python-version == '3.10'}}
+        uses: codecov/codecov-action@v3


I disagree, if my code contains platform- or Python version-specific logical branches, I exactly want to upload coverage multiple times so that all branches are covered.

i hear you! this guide is really focused on pure python builds and this tutorial needs to be beginner friendly! I think those newer to packaging likely need a simple point of entry BUT it's important to point out more complexity could be on the horizon! so i just edited the comment to say You only need to upload once unless you have a more complex build.

Fix: review edits from Jonny p2

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/ci.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/ci.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/ci.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/ci.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/ci.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/run-tests.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com> Fix: edit from trevor Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com> Fix: edit from trevor Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com> Fix: edits from Trevor Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Fix

Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>

Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Update ci-tests-data/ci.md Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Update ci-tests-data/ci.md Co-authored-by: Carol Willing <carolcode@willingconsulting.com>

Fix: navigation

Co-authored-by: Jeremy Paige <ucodery@gmail.com> Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

lwasser · 2024-01-09T02:08:30Z

Was there a larger discussion on discourse around promoting nox? I didn't see anything on Slack. It's, of course, a fine choice and I'm very late to the party, but I'm personally against nox for the simple reason that I need to write Python code that I may introduce bugs in rather than simply configuring the tool as I would for tox, for example.

Gosh @Midnighter that's a really good question. i think we talked about nox very early on but i'd have to dig into that discussion to find it. Essentially what is nice about nox is the configuration is python based so it's less cognitive overload vs learning tox. i definitely found it to be a lot more user friendly. so we did decide to recommend nox but that doesn't mean people might prefer to use tox (or other tools for that matter). some still use makefiles! a few benefits:

nox is great if you want to quickly run things locally in conda and venv envts
nox is being used by many core packages (including pip or it was when we were discussing it !)
nox is really flexible and allows you to automate many things beyond just tests (live docs build etc) and all using python code which is familiar

i think those are the big takeaways. I was able to get up and running with nox super fast (i moved from make) and it's also been great to run things like mypy across python versions and such (my personal take from our maintainer experience!). it just has been a huge time saver in my experience. :)

willingc

Looks good!

tests/run-tests.md

tests/tests-ci.md

willingc · 2024-01-10T21:17:15Z

conf.py

@@ -133,7 +133,7 @@


 # Social cards
-ogp_site_url = "http://www.pyopensci.org/python-package-guide"
+ogp_site_url = "http://www.pyopensci.org/python-package-guide/"


@lwasser Maybe use https

conf.py

ucodery · 2024-01-11T18:20:19Z

tests/run-tests.md

+set up virtual environments, and run tests across Python versions using the environment manager of your choice with a
+single command.
+
+:::{note} Nox Installations


I was thinking about something to notify the reader that nox doesn't need to be installed for the target python versions in order to manage them (as opposed to the venv, which must be present for each python version under test)

good point. i'm not sure if a beginner would get confused with that additional information or whether it would be useful. i hadn't thought about that before because nox just always worked! if you have suggestions for how to word it in a way that it doesn't make the user more confused i'm open to that!

tests/run-tests.md

ucodery · 2024-01-11T19:04:39Z

tests/run-tests.md

+    """
+
+    # Install dev requirements
+    session.install(".[tests]")


According to nox, session.install will always run pip. For conda it is recommended to use session.conda_install (there isn't a mamba_install, I assume conda_install works for both). I'm not sure what the best practice for installing a package under test in a conda environment is, but at least the package's dependencies should be installed via conda_install.

fixed! i missed the conda_install method in the docs!

tests/run-tests.md

tests/tests-ci.md

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>

Co-authored-by: Jeremy Paige <ucodery@gmail.com>

Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Co-authored-by: Jeremy Paige <ucodery@gmail.com>

tests/run-tests.md

Co-authored-by: Alex Batisse <alexandre.batisse@hey.com>

lwasser · 2024-01-16T17:30:31Z

ok everyone! i've fixed / responded to all suggested changes and this is now ready to merge!! it should be live in the next 5-10 minutes! 🚀

lwasser · 2024-01-16T17:31:16Z

i'm also confused as to why htmlproofer is passing given we have new pages here but we shall see!

lwasser force-pushed the run-tests branch from d86ccda to c5f3021 Compare January 3, 2024 22:45

lwasser added the 🚀 ready-for-review label Jan 3, 2024

lwasser changed the title ~~[Review round 2] Enh: Add tests pages - run tests in ci and locally to guide~~ [Review round 2 - review by jan 15] Enh: Add tests pages - run tests in ci and locally to guide Jan 3, 2024

ucodery reviewed Jan 4, 2024

View reviewed changes

Midnighter reviewed Jan 8, 2024

View reviewed changes

lwasser force-pushed the run-tests branch from c5f3021 to 88e5c8d Compare January 8, 2024 22:03

lwasser and others added 18 commits January 8, 2024 15:25

Fix: add initial tests content to guide

16734fb

ENH: fixes from Jonny's review

67351a1

Fix: review edits from Jonny p2

Fix: typos and cleanup

bc6a007

Fix: add example to tests ci page

1b9d232

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

a55ab25

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Fix: edits from @namurphy to run tests page

cdd2c09

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu> Update ci-tests-data/run-tests.md Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Fix: Rebase from main

0e6f7d4

remove header file

10db00d

Fix: conf.py cleanup

c4b5422

Fix: edits from Nick

71a382e

Co-authored-by: Nick Murphy <namurphy@cfa.harvard.edu>

Packaging image

d41b4fe

Fix: remove unused block

8b28c84

Fix

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

53b1818

Co-authored-by: Trevor James Smith <10819524+Zeitsperre@users.noreply.github.com>

Fix: edits from @willingc to ci page

838fbd0

Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Update ci-tests-data/ci.md Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Update ci-tests-data/ci.md Co-authored-by: Carol Willing <carolcode@willingconsulting.com>

Enh: Next two tests pages in guide

ace9515

Fix: navigation

Fix: spacing issue in conf.py during rebase

53dc089

lwasser force-pushed the run-tests branch from b47d8ce to 53dc089 Compare January 8, 2024 22:25

lwasser and others added 2 commits January 8, 2024 16:24

Fix: Edits from @ucodery

3d55eab

Co-authored-by: Jeremy Paige <ucodery@gmail.com> Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

Fix: Edits from @Midnighter

0455fa7

Co-authored-by: Moritz E. Beber <midnighter@posteo.net>

Fix: edits from review

0960184

lwasser force-pushed the run-tests branch from 279d05f to 0960184 Compare January 9, 2024 02:12

Fix: style tables and remove old test table image

794fa9d

lwasser force-pushed the run-tests branch from c9bddec to 794fa9d Compare January 10, 2024 00:29

lwasser added 5 commits January 9, 2024 17:30

Merge branch 'main' into run-tests

bf513c4

Fix: open graph tests

ce9437f

Fix: add random title

116d01e

Fix: updated card graphic for package guide

14deffc

Still fighting with opengraph

fb63a37

willingc approved these changes Jan 10, 2024

View reviewed changes

willingc reviewed Jan 10, 2024

View reviewed changes

lwasser added 2 commits January 10, 2024 17:49

Fix: head metatags update

c36e7f7

Merge branch 'main' into run-tests

8764ba6

lwasser commented Jan 11, 2024

View reviewed changes

conf.py Outdated Show resolved Hide resolved

Fix: card year should be https

84df946

willingc approved these changes Jan 11, 2024

View reviewed changes

ucodery reviewed Jan 11, 2024

View reviewed changes

willingc reviewed Jan 12, 2024

View reviewed changes

tests/tests-ci.md Show resolved Hide resolved

lwasser and others added 4 commits January 12, 2024 08:09

Fix: review edits from @willingc

9f75135

Co-authored-by: Carol Willing <carolcode@willingconsulting.com>

Fix: guide review from @ucodery

0cef5df

Co-authored-by: Jeremy Paige <ucodery@gmail.com>

Fix: edits from @willingc , @ucodery

4a99523

Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Co-authored-by: Jeremy Paige <ucodery@gmail.com>

Fix: edits from @WillInc @ucodery

2442e98

Batalex requested changes Jan 15, 2024

View reviewed changes

tests/run-tests.md Outdated Show resolved Hide resolved

tests/run-tests.md Show resolved Hide resolved

lwasser and others added 2 commits January 16, 2024 09:56

Fix: Edit from @Batalex

4152451

Co-authored-by: Alex Batisse <alexandre.batisse@hey.com>

Fix: a few last edits from the review

0d28e06

lwasser added enhancement-feature something new to add to our guide and removed 🚀 ready-for-review labels Jan 16, 2024

lwasser changed the title ~~[Review round 2 - review by jan 15] Enh: Add tests pages - run tests in ci and locally to guide~~ Enh: Add tests pages - run tests in ci and locally to guide Jan 16, 2024

lwasser merged commit 9d7ef13 into pyOpenSci:main Jan 16, 2024

lwasser deleted the run-tests branch January 16, 2024 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enh: Add tests pages - run tests in ci and locally to guide #145

Enh: Add tests pages - run tests in ci and locally to guide #145

lwasser commented Jan 3, 2024 •

edited

Loading

ucodery Jan 4, 2024

lwasser Jan 9, 2024

lwasser Jan 10, 2024

Midnighter left a comment

Midnighter Jan 8, 2024

Midnighter Jan 8, 2024

lwasser Jan 9, 2024

Midnighter Jan 9, 2024

Midnighter Jan 8, 2024

lwasser Jan 9, 2024

Midnighter Jan 8, 2024

lwasser Jan 9, 2024

lwasser commented Jan 9, 2024

willingc left a comment

willingc Jan 10, 2024

ucodery Jan 11, 2024

lwasser Jan 12, 2024

ucodery Jan 11, 2024

lwasser Jan 12, 2024

lwasser commented Jan 16, 2024

lwasser commented Jan 16, 2024

Enh: Add tests pages - run tests in ci and locally to guide #145

Enh: Add tests pages - run tests in ci and locally to guide #145

Conversation

lwasser commented Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Midnighter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwasser commented Jan 9, 2024

willingc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwasser commented Jan 16, 2024

lwasser commented Jan 16, 2024

lwasser commented Jan 3, 2024 •

edited

Loading