[Koha-devel] Good enough?

Mon Dec 5 15:35:02 CET 2022

 From what Julian write, I don't think he's suggesting running jenkins 
every type a patch is attached, but more something where a developper 
could send a message "hey, bot, can you check if this patch passes all 
tests ?". that would be manual (and I have no idea how to do that ;)
My 1 cent (not worth 2 ;) )

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/

-- 
Paul Poulain, Associé-gérant / co-owner
BibLibre, Services en logiciels libres pour les bibliothèques
BibLibre, Open Source software and services for libraries