Page 7 of 7

Re: Helping to review pull requests

Posted: Sat Jan 09, 2021 3:45 am
by chennes
DrInfiniteExplorer wrote: Thu Jan 07, 2021 4:35 pm I'm pretty sure that it's possible to write tests that performs interactions with Qt from python, to verify that the interface behaves as it should.
While it's possible to unit test GUI interactions, I will say that in my experience it's very difficult to do well, and very easy to do poorly.

If we look back to the very beginning of this discussion topic, it was basically a bunch of people on the forums asking to be added to the list of GitHub reviewers. Might we take the same approach to people who are allowed to commit to the Greenhouse? Have devs ask to be permitted to commit to the Greenhouse. Whoever maintains that list of people can set whatever standards they see as necessary (X number of forums posts, Y days since joining, Z accepted PRs, etc.), and can adapt as needed if there are problems. It adds a small amount of work, but might be enough to keep the greenhouse safe enough for the brave alpha tester types out there. (chrisb is right, I regularly compile and run code that I have never even looked at... in fact, I often do the code-review part of PR testing while the compile is running!)

Re: Helping to review pull requests

Posted: Sat Jan 09, 2021 4:01 am
by hyarion
chennes wrote: Sat Jan 09, 2021 3:45 am If we look back to the very beginning of this discussion topic, it was basically a bunch of people on the forums asking to be added to the list of GitHub reviewers. Might we take the same approach to people who are allowed to commit to the Greenhouse? Have devs ask to be permitted to commit to the Greenhouse.
I think this is a great idea.

Re: Helping to review pull requests

Posted: Sat Jan 09, 2021 9:54 am
by chrisb
chennes wrote: Sat Jan 09, 2021 3:45 am Whoever maintains that list of people can set whatever standards they see as necessary (X number of forums posts, Y days since joining, Z accepted PRs, etc.), and can adapt as needed if there are problems.
Good idea. It keeps the human factor, where something may be suspicious about a contributor which cannot really be grasped by a strictly automated procedure.

Re: Helping to review pull requests

Posted: Sun Jan 10, 2021 6:07 pm
by DrInfiniteExplorer
GeneFC wrote: Fri Jan 08, 2021 4:34 pm Obviously regressions are not good, but adding an extra burden to creating PRs may be worse.
We'd have to weight the burden of not being able to review, merge, and advance FreeCAD vs. rooting out hacky PRs that probably shouldn't be merged.
It should be noted that I am not advocating super stiff guidelines; If a reviewer or person with merge-rights think that a PR is mergeable without regression tests then that person should be able to decide to merge.
But having guidelines that advocate regression tests for PRs shouldn't go contrary to anything, and in time would help build up stability in FreeCAD and catch those PRs that would break things even if it might look they don't.
chennes wrote: Sat Jan 09, 2021 3:45 am While it's possible to unit test GUI interactions, I will say that in my experience it's very difficult to do well, and very easy to do poorly.
Agreed, but it's been shown it can be done properly, and from what I gather a lot of the overhead in merging PRs is from building and kinda aimlessly running FreeCAD and make sure nothing unexpected has broken.
Even just a small test-suite of smoke-tests that'd make sure common interaction in PartDesign works as intended would be a boon.



But as chenens wrote, this topic is diverging. What's the proper forum etiquette for splitting discussions into separate thread?
I'm not trying to step on anyones toes here but a lot of threads seem to diverge into separate issues without moderation. The root issue in this thread seems to have been addressed, and we've forked into regression test discussion and greenhouse discussion. That makes it hard to do follow-up and to determine if any concensus has been reached (kinda hard one either way?).
Yeah I see the irony in forking the thread again :roll:

Re: Helping to review pull requests

Posted: Sun Jan 10, 2021 10:56 pm
by GeneFC
DrInfiniteExplorer wrote: Sun Jan 10, 2021 6:07 pm
GeneFC wrote: Fri Jan 08, 2021 4:34 pm Obviously regressions are not good, but adding an extra burden to creating PRs may be worse.
We'd have to weight the burden of not being able to review, merge, and advance FreeCAD vs. rooting out hacky PRs that probably shouldn't be merged.
The problem that concerns me is the vast majority of PRs are quite small, sometimes just a one-line change. If there is a strong guideline that every PR needs heavy testing then a number of those fixes just would not happen.

Obviously there are many variations. I am not against more verification. I merely want to avoid throttling the small fixes.

Gene

Re: Helping to review pull requests

Posted: Sun Aug 15, 2021 6:33 am
by laschrocket
yorik wrote: Tue Jun 16, 2020 11:13 am It seems, contrarily of my first post, that not just anyone is allowed to make a review.. One has to be in a group on the github project. I'll create a "reviewers" group. If you want to help making reviews and are unable to do so on github, send me your github username and I'll add you.
could you please add me to the reviewers group, my github alias is klasrocket

Re: Helping to review pull requests

Posted: Sun Aug 15, 2021 4:18 pm
by Kunda1
laschrocket wrote: Sun Aug 15, 2021 6:33 am could you please add me to the reviewers group, my github alias is klasrocket
Yorik is out-of-the-office for the next week or so. Try pinging him next week. In the meantime you could add your feedback to the PRs directly or their associated forum threads (this is recommended).

Re: Helping to review pull requests

Posted: Sun Aug 15, 2021 6:57 pm
by laschrocket
Kunda1 wrote: Sun Aug 15, 2021 4:18 pm
laschrocket wrote: Sun Aug 15, 2021 6:33 am could you please add me to the reviewers group, my github alias is klasrocket
Yorik is out-of-the-office for the next week or so. Try pinging him next week. In the meantime you could add your feedback to the PRs directly or their associated forum threads (this is recommended).
Ok, thx for your update.

Re: Helping to review pull requests

Posted: Tue Aug 17, 2021 12:59 pm
by yorik
ok klasrocket you are added!
AFAIK this is not required anymore, and anyone can do a PR review?

Re: Helping to review pull requests

Posted: Fri Sep 24, 2021 6:15 pm
by sliptonic
yorik wrote: Tue Aug 17, 2021 12:59 pm ok klasrocket you are added!
AFAIK this is not required anymore, and anyone can do a PR review?
It's not necessary to do a review but it is necessary for a someone to request you to do one. It would be nice if we kept a list of reviewers and their interest/expertise somewhere. If I knew someone had UI skillz, only worked in Python, or was interested in Path for example, I'd request them to review certain appropriate PRs.