search

MDAnalysis / MDAKits

The MDAnalysis Toolkits Registry
https://mdakits.mdanalysis.org
Other
15 stars 24 forks source link

require minimal testing with instructions in metadata #95

Closed ianmkenney closed 2 weeks ago

ianmkenney commented 10 months ago

The current metadata.yaml template lists tests under "optional". This contradicts our latest paper.

orbeckst commented 10 months ago

That should be Mandatory — both in docs and the pydantic classes.

(Same goes for "install" and "install from source" I think, possibly different issue.)

IAlibay commented 10 months ago

Probably to be discussed here - what are do we consider to be the need here, is it "the registry should be able to run your tests", or "your mdakit should have some kind of regression test in place"?

The original intent of all this was the latter - i.e. you could have a script and some test data that made some assert and that would be ok. We can be stricter with the registry and say "we only accept things with tests", and that's ok too, but it does deviate from the loose "a script in a repo will be fine".

orbeckst commented 10 months ago

I consider "the registry should be able to run your tests" to be synonymous with "your mdakit should have some kind of regression test in place" for all intents and purposes. Regression tests, even if a script, are effectively useless if I cannot use them to decide if the package is broken. So the authors can be expected to write this script so that it returns a failure exit code when it internally determines that something didn't work. And they have to be able to tell us how to run their tests.

Even if there's a sliver of a difference between the two, I don't think we're doing anyone a favor if we start splitting hairs and distinguishing the two cases. In practice we do want kits to do what we're requiring so let's require it and document it.

If I had to review a registration request and these fields are not filled in, I would reject it because we are not able to do what we promised: continuously testing their code against MDA.

Finally: it's not that hard — the cookiecutter provides the infrastructure for anyone who wants to use it, including developers new to package building, and people who have a package ready should be able to figure this out, especially when Ian includes some notes on this in his additional write-up. Part of our intention is to educate developers so we are helping them to clear the bar, not lower the bar.

So, in summary: I am very much in favor of making run_tests and test_dependencies formally mandatory.

orbeckst commented 10 months ago

Related to #98 — just see discussion there.

orbeckst commented 2 months ago

I'd like to revive this issue together with #98 and come to a decision. Can we discuss at the next MDAKits meeting?

orbeckst commented 2 months ago

I put it on the agenda for the 2024-07-17 meeting.

orbeckst commented 1 month ago

At the EOSS4 MDAKits meeting on Jul 17, the MDAKits team decided that we want to make minimal tests and installation from source MANDATORY and thus enforce it during reviews.

In a subsequent round of consultations with the CoreDevs, there were a number of positive comments and no objections.

Therefore, minimal tests and installation from source instructions are MANDATORY and enforceable during review.

IAlibay commented 2 weeks ago

I believe this has been updated in the docs so I'm closing, but please re-open if there are any pending actions.