[Koha-devel] Good enough?

[Jonathan Druart]

I don't think we should run the whole test suite everytime we attach
patches, that would be very expensive in terms of resources.
However it would be interesting to have a temporary 'master' branch
that would become 'master' only if jenkins is happy.
"master" would never be broken :D

Using koha-testing-docker it's very easy to run tests locally.
It will be even more easier with
https://gitlab.com/koha-community/koha-misc4dev/-/issues/58

Le ven. 2 déc. 2022 à 18:16, Julian Maurice
<julian.maurice at biblibre.com> a écrit :
>
> Hi,
>
> It seems to me that several issues could be solved by having the CI run
> sooner, so authors can have feedback as soon as possible. Something like
> github's CI integration with pull requests would be amazing to detect
> common mistakes early and stop wasting human time. We should know if
> tests pass before pushing to master, not after.
> We could check tidyness, commit message conventions, code coverage by
> tests, ... all before another person have to look at it.
>
> Also tests are not easy to run locally. They might pass on Jenkins, but
> they do not on my local setup, so they are basically useless to tell me
> if I broke something. It also makes writing new tests more difficult
> than it should.
>
> If I wanted to improve Koha developer experience, I would start with that.
>
> My 2 cents.
>
> Le 02/12/2022 à 15:42, Jonathan Druart a écrit :
> > Hi devs,
> >
> > I was wondering... How good is your "good enough"?
> > It's a question I often ask myself, when writing patches or QAing
> > yours: is it good enough to be into Koha? It does not have to be
> > perfect or it may never reach master, but it must meet certain
> > standards.
> >
> > When I was RM I tried to put that limit quite high. Not necessarily by
> > asking the author for improving the follow-up patches, but also by
> > adding the missing bits myself.
> >
> > There are various types of "good enough", depending on what we look
> > at: good enough to be reviewed, good enough to be tested, to be put in
> > production, get feedback, try an idea, etc.
> >
> > Most of the time, what is driving the limit is answering  the
> > questions "Is it maintainable?" / "Is it future proof?". Making sure
> > the code you are writing won't be broken inadvertently is very
> > important and must be part of the reflection.
> >
> > Katrin asked the QA team members what were our plans for 23.05. In my
> > opinion we should enforce this "be future proof" concept. Writing code
> > is easy, especially in Koha (yes it is!). Writing maintainable and
> > robust code, following our guidelines is a bit harder. That's why we
> > have a QA process that tries to catch inconsistencies or edge cases
> > you may have missed.
> > But I think we can be even more rigorous, and aim for better
> > standards. We can elevate our ambitions and do better. When we see the
> > number of new enhancements we are releasing every 6 months, it shows
> > well that writing code is definitely not a problem. However sometimes
> > developers are tempted to abandon their work once they are pushed to
> > master. Pushed does not mean "done". Providing bug fixes, following-up
> > with patches when needed, fixing CI jenkins, etc. is part of job
> > (/fun)
> >
> > As a Koha developer for a long time now, I know how frustrating it can
> > be to be asked for follow-ups/rewrite/tests to have our patches
> > stamped with the precious PQA mark. But from the other point of view
> > (RM, RMaints, QA team), I also know it's very frustrating when you are
> > in charge of the release and you do not get the appropriate follow-up
> > work once it's pushed to master.
> >
> > There are some easy steps to write/review better patches. All have
> > been discussed already several times, but that can be enforced even
> > more:
> > 1. Perltidy (!) This is really a very trivial step. Please perltidy
> > your code. There are hundreds of commits that have been pushed in the
> > last months that are not tidy (alignment, indentation, lines too long,
> > etc.) This can easily be configured in your IDE! [1]
> >
> > 2. Provide clean code. As said it's not necessarily easy, but the QA
> > team and RM are supposed to know if the code is clean regarding Koha
> > guidelines. If the code is not clean, don't PQA, don't push. Either
> > clean yourself, or ask the original author of the patch to do it
> > (explaining to them how it can be improved ofc).
> >
> > 3. Squash! I have been away for a couple of months and had to read the
> > git history to know what I missed. And it was really hard to follow
> > what was going on. First of all, we are not consistent: the commit
> > message must tell what the patch is doing, not what the bug was (if
> > you are writing a bug fix). Then, there are way too many follow-ups:
> > tidiness, indentation fix, typo, spelling, etc. All those tiny
> > follow-ups could be squashed into the original patch. We don't need
> > unnecessary tons of entries in our git log for that. For instance, I
> > usually add a "JD Amended patch: perltidy" for instance when I tidy
> > the original patch, to keep track of the modification. Squash can be
> > done by the original author, the QAer, the RM. So yes, you are losing
> > one commit in the stats but the git log is easy to read!
> > We could have an "Amended-by" marker if we really want to add credit
> > on the dashboard (and/or release notes).
> >
> > 4. Run tests. Don't wait for Jenkins to fail. This is valid for the
> > author and QA. Anticipate the failures by running more tests. If you
> > are modifying C4::Circulation, then run prove on
> > t/db_dependent/Circulation*, not only Circulation.t. It will help you
> > catch edge cases.
> > When something is pushed, track down jenkins failures that could be
> > caused by your patches.
> >
> > 6. Be strict if you are QAing. Each QA member has their own "good
> > enough", and the RM as well (either relying on the QAer or providing a
> > full review). But QA must fail if the code is old Koha style code, or
> > not "good enough".
> >
> > 7. Provide support for failing tests, fix things you broke. The QA
> > team will be more comfortable with your patches if you show them you
> > are providing support for your stuff.
> > It's not because it's pushed that you don't have any more efforts to
> > make. Provide follow-up patches you promised, provide bug fixes, etc.
> > We don't have a good way to keep track of such demands, which does not
> > make tracking easier for devs, QA and RM. Any suggestions?
> >
> > 8. QA team MUST NEVER* pass QA a change that is not covered by tests,
> > never. You should not provide change to modules without tests!
> > * almost never...
> >
> > 9. Stick to existing patterns. We should not have different ways to do
> > the same thing. We should not have different places where a code is
> > doing the same thing. Ask for help or advice on the list or IRC before
> > you start coding. We will be happy to guide you. Even if you are a
> > regular Koha developer it's not always easy to be aware of the latest
> > master changes.
> > We will tell you what's the current good practice, or point you to
> > examples you could reuse for what you want to implement.
> >
> > 10. CI should drive the pushes. No more push if CI is not green. The
> > more we wait the harder it is to track down the origin of the problem.
> > Last cycle some jobs have been red for months, and we released
> > 22.11.00 with D10, D11, D12 marked unstable...
> >
> > What will I do next cycle?
> > All of that, and more. I will track down jenkins failures and
> > responsibilize developers telling them when they break tests (and
> > won't fix them anymore as I have been doing for years).
> > I will raise on the bug reports what could have been improved. Yes,
> > read that I will be even more annoying (to put it politely) than
> > before.
> >
> > I've noticed that the pre-commit git hook on the wiki has been broken
> > for more than 3 years. And also caught some core developers that do
> > not have it in place. I am relying on it to keep Vue files tidy so
> > it's important to have it set up properly. I am planning to force its
> > usage for ktd users [2]. Adding more checks to it will help us to
> > catch inconsistencies from the beginning.
> >
> > To summarize, writing code is cheap, maintaining code is way more
> > expensive! It is easier to get the attention of developers before the
> > patches are pushed to master than after, so we could be more ambitious
> > and ask more.
> >
> > For discussion :)
> >
> > Cheers,
> > Jonathan
> >
> > [1] If you are using vim, open ~/vimrc, add
> >    vmap <F8> :!perltidy -q<CR>
> > Reload vim, select code in visual mode
> > [2] https://gitlab.com/koha-community/koha-misc4dev/-/issues/59
> > _______________________________________________
> > Koha-devel mailing list
> > Koha-devel at lists.koha-community.org
> > https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> > website : https://www.koha-community.org/
> > git : https://git.koha-community.org/
> > bugs : https://bugs.koha-community.org/
>
> --
> Julian Maurice
> BibLibre
> _______________________________________________
> Koha-devel mailing list
> Koha-devel at lists.koha-community.org
> https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> website : https://www.koha-community.org/
> git : https://git.koha-community.org/
> bugs : https://bugs.koha-community.org/