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

Unitize/dedupe tests #60

Merged
merged 4 commits into from
Nov 1, 2024

Conversation

sneakers-the-rat
Copy link
Contributor

@sneakers-the-rat sneakers-the-rat commented Oct 30, 2024

Sorry for a bigger pr, these things are sorta all related and would be hard to do piecemeal


I was surprised at how long the tests took to run!

Here were all the tests with params that were running (sorta sorted because for some reason pytest was also running them out of order)

tests/test_template_init.py::test_init_template[GitHub-Apache-2.0-] PASSED        
tests/test_template_init.py::test_init_template[GitHub-Apache-2.0-sphinx] PASSED  
tests/test_template_init.py::test_init_template[GitHub-Apache-2.0-mkdocs] PASSED  
tests/test_template_init.py::test_init_template[GitHub-BSD-3-Clause-] PASSED      
tests/test_template_init.py::test_init_template[GitHub-BSD-3-Clause-sphinx] PASSED
tests/test_template_init.py::test_init_template[GitHub-BSD-3-Clause-mkdocs] PASSED
tests/test_template_init.py::test_init_template[GitHub-MIT-] PASSED               
tests/test_template_init.py::test_init_template[GitHub-MIT-mkdocs] PASSED         
tests/test_template_init.py::test_init_template[GitHub-MIT-sphinx] PASSED         
tests/test_template_init.py::test_init_template[GitLab-Apache-2.0-] PASSED        
tests/test_template_init.py::test_init_template[GitLab-Apache-2.0-sphinx] PASSED  
tests/test_template_init.py::test_init_template[GitLab-Apache-2.0-mkdocs] PASSED  
tests/test_template_init.py::test_init_template[GitLab-BSD-3-Clause-sphinx] PASSED
tests/test_template_init.py::test_init_template[GitLab-BSD-3-Clause-mkdocs] PASSED
tests/test_template_init.py::test_init_template[GitLab-BSD-3-Clause-] PASSED      
tests/test_template_init.py::test_init_template[GitLab-MIT-] PASSED               
tests/test_template_init.py::test_init_template[GitLab-MIT-mkdocs] PASSED         
tests/test_template_init.py::test_init_template[GitLab-MIT-sphinx] PASSED         

tests/test_template_init.py::test_template_suite[GitHub-] PASSED                  
tests/test_template_init.py::test_template_suite[GitHub-sphinx] PASSED          
tests/test_template_init.py::test_template_suite[GitHub-mkdocs] PASSED            
tests/test_template_init.py::test_template_suite[GitLab-] PASSED                  
tests/test_template_init.py::test_template_suite[GitLab-sphinx] PASSED            
tests/test_template_init.py::test_template_suite[GitLab-mkdocs] PASSED            

Unit-izing

(is there a better word for "making a test more of a unit test?" i don't know a better one lol)

The test_template_init tests are fine, and that should be a combinatoric test because it's a) fast-ish, and b) a baseline test for "does this option work at all"

The test_template_suite tests were the thing that was taking a long time. For each of those parameterizations, every hatch env was being installed and each command was being run, but most of those tests don't vary by the parameterization. e.g. the docs:build script was being run twice for each docs framework even though nothing meaningfully varies in the docs by the git provider, as far as I can tell. This would become a problem p quickly if we were to add more git remotes (which i plan to in a sec) or different documentation modes (the mkdocs env is marked as "mkdocs-material" so i assume other skins/themes are a potential?)

Now don't get me wrong, i'm all in favor of ~ rigorous testing ~, but i am also a believer in "tests that run fast are run more often," and also to the degree we can make our tests unit-ized and testing one small thing, we should. Both for maintenance's sake but also for developer experience's sake - e.g. i added a mark for tests that relate to docs, so if i changed something about how docs worked, i can just run all the tests that relate to docs quickly like pytest -m docs and so on.

Now we can add the parameterizations back to the test_template_suite if we need to - y'all know the template better than I do and what is needed, i just got here - but by splitting off some pieces of it we get finer grained control over which and how tests are parameterized :). e.g. Ideally we would make a new test module for just running the scripts in a generated package, and each test function tests a single script over the parameterization that is relevant to that script, but didn't want to go that far yet!

Independent-izing

I see that we were using tmp_dir_factory and generating into subdirectories according to parameterizations? It looks like that was to re-use hatch envs, but i could be wrong. Problem with that is that now the tests are no longer independent! Copier doesn't clean the target directory by default, so files that are already there are left there. That's a problem e.g. for the test_init function, where we vary the destination directory by dev_platform and license, but not by documentation, and since that test just looks for the presence of files, if one of the documentation params broke something, we might not know! edit: forgot about the numbered param that autoincrements, whoop. nevermind this part!

So i swapped that out with just plain tmp_path which is a function-scoped fixture that makes a new dir for teach invocation of a test rather than tmp_dir_factory which is session-scoped. If we want to have a custom output directory fixture function (which is totally valid and i actually do this in all my projects that generate output so that it's easier for people to submit samples of bugs by zipping up the tmp dir), i can add that back in, but i do think each test should be independent! this will also be necessary if we want to parallelize the tests :)

De-boilerplating

I noticed as i was splitting out the tests here that i was wanting to copy and paste the setup steps to run copier, which is a telltale sign it's time for a fixture or a helper :). So I made a helper function to init the generated project as a repo, and a fixture closure (because it uses the answers fixture for defaults) to generate a project with default args, except overriding with any kwargs that are passed. Together they make a tidy lil way to test variations on passed args.

Mark-ing

(ok marking is just a normal word it's just a pattern i can't help myself)

one last tiny thing, i also marked all the tests that install packages so that they could be excluded if needed - since this is a template generating package it's super normal and good to install packages and do network things during the tests, but it's also ncie to be able to turn that off since network-bound tests can be flaky and slow and use up disk space. that's just a convenience thing tho. atm most of the tests install packages, but i'm gonna be adding mostly non-network tests to like confirm our settings are correct as i go so it will make more sense later. Hopefully we only need a few end-to-end tests where we actually run the generated package and can mostly just validate the test output for correctness :)

@sneakers-the-rat
Copy link
Contributor Author

notte bad - from 16m down to 6m: https://github.com/pyOpenSci/pyos-package-template/actions/runs/11603374569 , and the ubuntu and mac tests finish in 1m

Midnighter
Midnighter previously approved these changes Oct 31, 2024
Copy link
Contributor

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

I love it, thank you so much! We had definitely accrued a decent amount of tech debt already, so I appreciate you cleaning house.

tests/test_template_init.py Outdated Show resolved Hide resolved
@sneakers-the-rat
Copy link
Contributor Author

I am new to this project and dont know norms on merging, is someone the merge Queen or are we just going for it once we get approving reviews?

@Midnighter
Copy link
Contributor

You can have the pleasure of merging yourself =]

@sneakers-the-rat
Copy link
Contributor Author

ok except it looks like i accidentally did something to dismiss your review lol could i get another @Midnighter

@sneakers-the-rat sneakers-the-rat merged commit 8c1f1c1 into pyOpenSci:main Nov 1, 2024
6 checks passed
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.

2 participants