Helping to review pull requests

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
chennes
Posts: 249
Joined: Fri Dec 23, 2016 3:38 pm
Location: Norman, OK, USA
Contact:

Re: Helping to review pull requests

Postby chennes » Sat Jan 09, 2021 3:45 am

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
hyarion
Posts: 54
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Postby hyarion » Sat Jan 09, 2021 4:01 am

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
Posts: 30821
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Postby chrisb » Sat Jan 09, 2021 9:54 am

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.
You need at least FreeCAD 0.19.23300 to edit my current sketches.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
DrInfiniteExplorer
Posts: 37
Joined: Wed Dec 30, 2020 2:16 am

Re: Helping to review pull requests

Postby DrInfiniteExplorer » 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.
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
Posts: 1805
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: Helping to review pull requests

Postby GeneFC » Sun Jan 10, 2021 10:56 pm

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