Helping to review pull requests

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
chennes
Veteran
Posts: 3879
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Helping to review pull requests

Post 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!)
Chris Hennes
Pioneer Library System
GitHub profile, LinkedIn profile, chrishennes.com
hyarion
Posts: 139
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Post 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.
chrisb
Veteran
Posts: 53930
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Post 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.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
DrInfiniteExplorer
Posts: 39
Joined: Wed Dec 30, 2020 2:16 am

Re: Helping to review pull requests

Post 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:
GeneFC
Veteran
Posts: 5373
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: Helping to review pull requests

Post 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
laschrocket
Posts: 11
Joined: Fri Dec 27, 2019 3:54 pm

Re: Helping to review pull requests

Post 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
User avatar
Kunda1
Veteran
Posts: 13434
Joined: Thu Jan 05, 2017 9:03 pm

Re: Helping to review pull requests

Post 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).
Alone you go faster. Together we go farther
Please mark thread [Solved]
Want to contribute back to FC? Checkout:
'good first issues' | Open TODOs and FIXMEs | How to Help FreeCAD | How to report Bugs
laschrocket
Posts: 11
Joined: Fri Dec 27, 2019 3:54 pm

Re: Helping to review pull requests

Post 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.
User avatar
yorik
Founder
Posts: 13640
Joined: Tue Feb 17, 2009 9:16 pm
Location: Brussels
Contact:

Re: Helping to review pull requests

Post by yorik »

ok klasrocket you are added!
AFAIK this is not required anymore, and anyone can do a PR review?
User avatar
sliptonic
Veteran
Posts: 3457
Joined: Tue Oct 25, 2011 10:46 pm
Location: Columbia, Missouri
Contact:

Re: Helping to review pull requests

Post 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.
Post Reply