Re: [Koha-devel] Good enough?
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
[Koha-devel] Good enough?
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
