edbennett / python-testing-ci

https://edbennett.github.io/python-testing-ci
Other
2 stars 3 forks source link

CI vs. pre-commit #25

Open chillenzer opened 1 year ago

chillenzer commented 1 year ago

I noticed that we talk a bit about CI but not at all about pre-commit. They serve different purposes and not necessarily imply each other but for my personal work I rely significantly more on pre-commit than on CI (because whatever CI would be running is already ran by pre-commit). I'm aware that CI is probably the more important part, so we don't need to discuss that but should we add (at least a mentioning of) pre-commit somewhere?

edbennett commented 1 year ago

Yes, mentioning it is a good idea. Pre-commit itself discourages running the test suite in pre-commit, but other checks definitely are worth mentioning.

chillenzer commented 1 year ago

Really? Why? Where?

edbennett commented 1 year ago
chillenzer commented 1 year ago

Well, I think it is a problem of defaults and software engineering skills. I think the default should be to have all tests running in pre-commit and have a (unit-) test suite that runs in very few seconds. The latter is the software engineering skill of keeping the unit tests small and to the point, i.e. finding minimal test cases that require very little resources (the book example in the lesson is a particularly bad example for that) and testing enough but not excessively. If despite all efforts, this becomes unacceptably large, then one should first define a subset of the tests that is fast enough to still run every time, find a mechanism to only run affected tests and only after both of the previous are not enough any more disable tests in pre-commit again. I'm confident that most of the people we teach won't hit that point with their software really.

chillenzer commented 1 year ago

Finally, it's personal preference that you could manually switch off on your machine if you don't like it.

chillenzer commented 1 year ago

Okay. After reading a bit more about this, the pre-commit maintainers seem to be pretty annoyed by this discussion by now. Mmmmhhh... it's probably something that is really dependent on the project size. But using TDD a lot, I usually run my test suite multiple times even before committing and it has saved me a lot of trouble when pre-commit still found a failing test.

edbennett commented 1 year ago

The pre-commit dev is rather opinionated, and this creates some QOL issues in some areas. (E.g. the repeated argument over whether it's reasonable to demand us to define our requirements in two separate places, one just for pre-commit.)

Personally I have yet to encounter a codebase with a non-trivial test suite that runs in a reasonable amount of time, so I don't think saying it's a skill issue is helpful.

chillenzer commented 1 year ago

Okay. That might be my lack of experience. Will report back in ten years. =)

chillenzer commented 1 year ago

PS: I said that it is a skill to keep them short but not that it is a lack of skill if it still gets out of hands at some point. That skill will only help you in pushing this boundary a little further which is all I advocated for. Again, I'm talking about the specific use case of our learners probably having relatively small projects (for starters).

edbennett commented 1 year ago

Many students on the CDT end up contributing to larger projects (e.g. HiRep, Bilby, ...) that will have large test suites.

And even if not, if possible I'd like to avoid a situation where a student adds more tests to their work and pushes up the runtime of the test suite such that

a. they think "oh no, I'm an awful programmer and a terrible person and don't deserve to be here", or b. they remove tests to make the test suite faster and therefore "better" c. they disable pre-commit altogether

(Also I'd be cautious about giving advice directly contradicted by the authors of a tool until learners have more experience in the area.)