feat: add snapshot tests with syrupy

[jandom]

I really want to refactor this library a bit: everything is based on inheritance (as is most MDA) and it's basically impossible to read it can be challenging for a newcomer to get started. To refactor safely, we need to have a test harness to ensure that nothing is broken.

There is a number of python scripts in

python "${GENPATH}/gen_format_overview_classes.py"
python "${GENPATH}/gen_selection_exporters.py"
python "${GENPATH}/gen_standard_selections.py"
python "${GENPATH}/gen_topology_groupmethods.py"
python "${GENPATH}/gen_topologyattr_defaults.py"
python "${GENPATH}/gen_topologyparser_attrs.py"
python "${GENPATH}/gen_unit_tables.py"

And the fastest way to get these under tests is by adding snapshot tests. The tests in this PR exercise all the python code in this codebase.

All the .ambr files are automatically generated via

python -m pytest --snapshot-update 

[jandom]

Bonus finding here is that creating the snapshot tests, allowed code coverage to be generated, and identified a number of code paths that are never executed. Simpler code is always easier to refactor, but that will be a follow-up PR – out of scope here

[lilyminium]

Hi @jandom, thanks for opening this PR. The user guide was meant to be documentation and the scripts just a way to ensure that changes in the disconnected core repo were reflected in the user guide with minimal effort -- as such, the code in the package are just fairly quick-and-dirty scripts that are not very robust; tests and refactoring would be a huge improvement.

However, I'm not familiar with the syrupy library. Is the idea to compare the output of the functions to previously generated snapshots? If so, does that mean anything new that results in a change (e.g. the new TNG change, or possibly changes in core MDA library) would result in needing to re-generate snapshots?

[jandom]

hi there @lilyminium, my apologies for being critical – I should have been more positive. I deeply care about code quality, especially in research. MDA is the leading package in this ecosystem, where we set the bar, others pay attention. The easier the code is to maintain, the lower the bar to entry to folks from all walks of life.

Onto syrup, you are correct – it's just dumb snapshot test library. If the output differs from a previous output, error. These snapshot tests are very brittle and very powerful for certain uses: like ensuring that a refactor didn't break anything (where as unit tests you'd have to adjust heavily). I'm not advocating there for including snapshot testing into CI/CD but could be useful for detecting drift in refactors, and applicable for detecting dead code.

I really want to cleanup this code (tests, typing, all that jazz) as an example for what's possible. But to do that I want to make super-sure I don't break anything by accident.

[jandom]

Here are some examples of detected dead-code for example, in base.py, removing this code makes refactoring so much simpler. "Does this corner case ever happen? Nope, not really"

[jandom]

we don't have to do this – this will be very brittle, anytime an output changes, we'll have to re-generate the snapshots. helpful to document how to run the snapshot tests however, so maybe we can leave it with "allow to fail"

[lilyminium]

I would say leave it for now -- we have scheduled twice-weekly tests that should pick up changes in the core repo in suitable time so we can update snapshots as needed.

[lilyminium]

@jandom, apologies if I came off as negative -- I was just explaining a bit about the history and context of why these scripts are the way they are. I totally agree with your opinion and your assessment of the code in the user guide! You've been doing fantastic work fixing and cleaning it up and we deeply appreciate everything you've done.

Refactoring the scripts would hugely improve readability and maintainability, and I agree that tests are necessary to ensure output doesn't change meaningfully. Wrt snapshots, my feeling is that the brittleness of snapshot tests may make them unsuitable for the CI pipeline; since the scripts pull from the MDAnalysis core repo, changes to the core library could alter the output here. Of course, this is also going to be an issue that would plague unit tests unless they were tested on toy input; and unit tests would take more effort to write.

I'm inclined to approve this for now to get things going, but to raise an issue with the goal of replacing the snapshot tests with unit tests for your refactored scripts as the ultimate long-term solution.

I'd also be interested in what @RMeli and @IAlibay think but they have a lot on their plates right now.

[RMeli]

@jandom sorry for the slow replies here there days. I associate with @lilyminium, your work on the UserGuide is very valuable and much appreciated!

Do I understand correctly that this snapshot testing would be only a temporary solution during refactoring? If yes, it is possible/would it make sense to pull from a fixed MDAnalysis version during the refactoring (2.5.0 or a specific commit)? In this way the snapshots would remain consistent.

[jandom]

@RMeli no problemo, all the reviews came today like an avalanche!

not sure how we could pin the MDAnalysis version used by snapshots, other than by pinning the version in requirements.txt and environment.yml, but then this repo wouldn't trail MDAnalysis automatically.

yeah, i would use snapshots for refactoring, for when the behavior shouldn't change.

[RMeli]

not sure how we could pin the MDAnalysis version used by snapshots, other than by pinning the version in requirements.txt and environment.yml, but then this repo wouldn't trail MDAnalysis automatically.

Right, I think it might need a separate environment.yaml file. But then we could leave such action and update the snapshots once per release, I think.

[IAlibay]

But then we could leave such action and update the snapshots once per release, I think.

I've not read anything but this but with my release manager hat on I'm going to block on this until I get a chance to review.

P.S. I'm swamped with irl things at the moment so it might take me a week at least.

[jandom]

nw @IAlibay this is not a huge rush – to be fair I'd expect you to be the major benefactor of this PR, because it'll signal to you if stuff "unexplainably" changes under the hood

[lilyminium]

Right, I think it might need a separate environment.yaml file. But then we could leave such action and update the snapshots once per release, I think.

I think pinning the version to the last release would make it quite difficult to update and release the user guide in tandem with the core library. If anything, trailing the core repo live and having snapshots potentially break means we can track how sustainable, disruptive, or useful snapshot tests could potentially be.

@jandom I see you've already opened a PR against this branch -- could that be a potential avenue forward for your refactor? i.e. to test against snapshots but not merge into develop until the refactor is done and potential unit tests could be put in?

[jandom]

We could leave the branch unmerged, yeah, but that will require a lot of git acrobatics – at least given my level of git ;-)

To address the concern from @IAlibay I'd maybe advocate for silent launching the snapshot tests – we merge the PR and permit the snapshot tests to fail. Whoever wants to pay attention, can, but release manager won't be blocked.

[RMeli]

I think pinning the version to the last release would make it quite difficult to update and release the user guide in tandem with the core library.

In case I wasn't clear, I wasn't suggesting a global pinning but a pinning only for the snapshot tests during their refactoring.

If anything, trailing the core repo live and having snapshots potentially break means we can track how sustainable, disruptive, or useful snapshot tests could potentially be.

But yeah, this is a good point!

[IAlibay]

But then we could leave such action and update the snapshots once per release, I think.

I've not read anything but this but with my release manager hat on I'm going to block on this until I get a chance to review.

P.S. I'm swamped with irl things at the moment so it might take me a week at least.

I've not forgotten about this - just want to get 2.6.0 out first and then I'll try to come back to it.

[jandom]

Sounds good, i'm on a little holiday so not looking at this too carefully

[jandom]

@IAlibay a gentle bump – i don't know if you're still busy (probably yes), maybe there is someone you could... khm-khm "volunteer" to do the review? :P

[IAlibay]

@IAlibay a gentle bump – i don't know if you're still busy (probably yes), maybe there is someone you could... khm-khm "volunteer" to do the review? :P

Sorry @jandom I started looking at this but I can't dedicate a ton of time for reviews right now. I'm not keen to unblock this until I get a really good idea of exactly how this impacts releases.

Maybe the fastest way to get this moving is if you could detail out how you anticipate things to affect releases (I'm sure it's detailed somewhere along the review but it'd be easier if it was concentrated in one comment).

[jandom]

Maybe the fastest way to get this moving is if you could detail out how you anticipate things to affect releases (I'm sure it's detailed somewhere along the review but it'd be easier if it was concentrated in one comment).

Gotcha. Let's consider two options

1. The main idea behind the snapshots is to provide a safety net during refactoring. We can generate them before refactoring and just double-check no unintended changes slipped through. No need to commit them to the repo unless we want to. No impact on releases either.
2. If we decide to fold them into the release process, it'd go like this

• Bump the MDA version.
• Run python -m pytest tests/snapshot/ --snapshot-update.
• Commit any updated snapshot files (though often, there might not be any changes).

It's all about giving us a bit more peace of mind with minimal fuss. Let me know what you think!

[jandom]

Bump, sorry just wondering about this PR again. What's the reservation with respect to the releases? That it will all break/get brittle at exactly the time when you need to pump the release out? I can probably understand that... the flip side being that there were no tests here at all – surely not having tests should be more scary to a release manager? ;-)

[IAlibay]

Sorry about the delay.

I think we'll eventually need to automate this, but for now could you add the proposed release process to https://github.com/MDAnalysis/UserGuide/blob/develop/doc/source/preparing_releases_and_hotfixes.rst under "release of the userguide"?

That way we know exactly what needs to happen.

[jandom]

Thank you for the review, I'll go ahead and update the docs

[pep8speaks]

Hello @jandom! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file doc/source/scripts/tests/snapshot/test_gen_unit_tables.py:

Line 14:80: E501 line too long (83 > 79 characters)

Comment last updated at 2023-10-08 19:31:02 UTC

[IAlibay]

cheers sorry about the delay here, it's continued to be a very busy year for MDAnalysis - bit more breathing room now that the UGM is over

[IAlibay]

I don't know if @lilyminium or @RMeli had anything else to say, but to me at least this satisfies the "I know that to do next time I do a release" problem.

[jandom]

Super happy we're moving ahead with this – and thank you for all the reviews. Love to see a zombie PR turned back alive ;-)