[Koha-devel] Good enough?

Mon Dec 5 18:40:20 CET 2022

Having the hooks installed automatically will help devs catch small
inconsistencies, that's a first step for the easy-to-catch things.

The run_tests.pl script in misc4dev will hopefully help to run the tests by
anybody.

Running the tests on a DB without the sample data or with existing data is
already a goal, it's something we are trying to catch during QA.
TestBuilder helped us a lot for that. Create the data the tests need and
the cleanup/rollback should be the way to write tests.

Le lun. 5 déc. 2022 à 18:24, Julian Maurice <julian.maurice at biblibre.com> a
écrit :

> Yes it is expensive. But manual testing is much more expensive. I'd
> rather pay for some CPU time than do manually what a bot can do better
> and faster than me. Free services exist too (CircleCI, github actions, ...)
>
> Pushing to a temporary master branch is not a bad idea (a never-broken
> master branch sounds nice), but I think it would happen too late.
> Patches would have already been rebased multiple times, tested and
> reviewed before we notice a test failure.
>
> koha-testing-docker feels more like a symptom of the difficulty to run
> tests than a satisfying solution. It is probably necessary in order to
> run complicated end-to-end tests, but it should not be mandatory to run
> simple unit tests.
> And ktd is not that easy to use. It can break, and it's not easy to
> debug for someone not familiar with docker.
>
> Don't you think it would be a lot easier if we could run `prove t` (or
> `npm test` or whatever) on any dev setup and have the same
> failures/successes as Jenkins (minus the complicated tests like those
> that require selenium) ?
> Of course making that happen is not an easy task, but it should be a
> long term goal IMO.
>
> Also I think no one wants to be the person that refuse a patch because a
> comment is misaligned, so if that kind of thing is not automated, "not
> good enough code" will continue to be pushed.
>
>
> Le 05/12/2022 à 14:55, Jonathan Druart a écrit :
> > 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/
> > _______________________________________________
> > 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/
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20221205/54bdfcdf/attachment-0001.htm>