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

Re: Helping to review pull requests

Post by yorik »

Werner will review anything in App, Gui, Part and PartDesign, and many in other areas too. He's probably the most skilled of us all, and by far the one with most FreeCAD experience as he's there since the very beginnings. So yes, his authority is unquestionable. But it's not true that he only trust himself at all. All the way FreeCAD grew up to know is based on trust between people.

But you're right we should write better docs to explain what is needed.

The main problem we have really is with large pull requests that touch many files. It's difficult or impossible to measure the impact of such changes only by looking at the code. So what's mostly needed there are people wanting to compile the branch from the pull request, check the functionality that the PR proposes (check that it correclty does what it is intended to do), and, that's the most important and the hardest part and that requires a fairly good knowledge of FreeCAD, give some stress-test in all areas possibly affected by the proposed changes.

For example, a PR that would propose changes to the tree view: What would need to be tested is that the changes are working correctly, but that other operations that use the tree view or redraw it are still working ok: adding objects, removing objects, right-clicking everywhere, selecting, embedding in parts and groups, switching workbenches, try to make possible problems appear.

This helps enormously to make the merging faster, because it takes some time (build,run, test...) and it's hard to automate and humans are far better at causing crashes than automated tests :D and if you see that someone else did all this and reports no error you can have much, much more trust in the PR already
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Helping to review pull requests

Post by openBrain »

yorik wrote: Mon Jan 04, 2021 2:17 pm Werner will review anything in App, Gui, Part and PartDesign, and many in other areas too. He's probably the most skilled of us all, and by far the one with most FreeCAD experience as he's there since the very beginnings. So yes, his authority is unquestionable.
It's not about authority (which indeed is unquestionable), it's about what he's ready to delegate with trust and what not.
But it's not true that he only trust himself at all.
I didn't say so. I said that it shall be told if it is the case. ;)
But you're right we should write better docs to explain what is needed.
Don't know if it's only about doc, but we must help people set their effort when it's useful and can have an impact.
The main problem we have really is with large pull requests that touch many files. It's difficult or impossible to measure the impact of such changes only by looking at the code. So what's mostly needed there are people wanting to compile the branch from the pull request, check the functionality that the PR proposes (check that it correclty does what it is intended to do), and, that's the most important and the hardest part and that requires a fairly good knowledge of FreeCAD, give some stress-test in all areas possibly affected by the proposed changes.
Let me propose another approach. Maybe it's time to consider the dev version as being a bit more unstable. Or if we want to keep a certain stability to dev version, time to consider creating a 'greenhouse' version where eg. all PR that pass CI test are merged. It can help growing the testing base by making it accessible to all users.
User avatar
voskos
Posts: 67
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

Re: Helping to review pull requests

Post by voskos »

May I suggest a industry accepted way to speed up merges is more automated testing.
chrisb
Veteran
Posts: 53928
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Post by chrisb »

voskos wrote: Mon Jan 04, 2021 9:28 pm May I suggest a industry accepted way to speed up merges is more automated testing.
The framework for this exists, however, it must be filled, not only recommended.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
User avatar
voskos
Posts: 67
Joined: Mon Dec 21, 2020 4:22 pm
Location: Greece

Re: Helping to review pull requests

Post by voskos »

Its 00:30 what doe
chrisb wrote: Mon Jan 04, 2021 10:30 pm The framework for this exists, however, it must be filled, not only recommended.
Its 00:30 here and I trying to count edges to see if refine=true did anything :lol: :lol: I am trying here, give me a break :D
Boost unit tests would be nice to have also :twisted: :lol:
chrisb
Veteran
Posts: 53928
Joined: Tue Mar 17, 2015 9:14 am

Re: Helping to review pull requests

Post by chrisb »

voskos wrote: Mon Jan 04, 2021 10:35 pm Its 00:30 here and I trying to count edges to see if refine=true did anything :lol:
:lol:
Can be hard, but the mere counting can be done by Part->CheckGeometry. However, that's not enough: I recently investigated a helix and refine cleaned indeed the existing edges. But at the same time new ones were introduced, probably because a cylinder always has to have a seam.
A Sketcher Lecture with in-depth information is available in English, auf Deutsch, en français, en español.
hyarion
Posts: 139
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Post by hyarion »

openBrain wrote: Mon Jan 04, 2021 2:52 pm Let me propose another approach. Maybe it's time to consider the dev version as being a bit more unstable. Or if we want to keep a certain stability to dev version, time to consider creating a 'greenhouse' version where eg. all PR that pass CI test are merged. It can help growing the testing base by making it accessible to all users.
This usually works for small teams where everyone can be trusted but for FreeCAD anyone can create a pull request.
It's easy to make a change that passes the CI tests but still does something horribly wrong and not everything is easy to cover in a unit test. Relaxing the review-constraint can be a slippery slope.

For example, ui-related and workflow changes are difficult to cover automatically but can have a big impact on the software.


It would be nice though, if we could find a way to make the review process easier where more people can (and dares to) accept pull requests.
User avatar
watsug
Posts: 100
Joined: Sat Sep 26, 2020 10:51 pm

Re: Helping to review pull requests

Post by watsug »

hyarion wrote: Tue Jan 05, 2021 1:50 am For example, ui-related and workflow changes are difficult to cover automatically but can have a big impact on the software.
I think that's why it's good to have a compiled dev/greenhouse version, so more people can download and try out new "experimental" changes and give feedback, without the need to compile themselves. Because those features need user feedback, and not only dev feedback (even though the overlap in freecad is big :) )
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Helping to review pull requests

Post by openBrain »

hyarion wrote: Tue Jan 05, 2021 1:50 am This usually works for small teams where everyone can be trusted but for FreeCAD anyone can create a pull request.

OK, we can say any user with already one contribution merged in the master will have its PR automatically merged in the greenhouse version.

It's easy to make a change that passes the CI tests but still does something horribly wrong and not everything is easy to cover in a unit test. Relaxing the review-constraint can be a slippery slope.

It's absolutely not relaxing. PR get the same care from mergers before being merged in dev/final. Just they have been already functionally reviewed by alpha users. So it's even better reviewed. ;)
hyarion
Posts: 139
Joined: Fri Jun 26, 2020 6:08 pm

Re: Helping to review pull requests

Post by hyarion »

Ah, my bad. I now understand your definition of a greenhouse version. That sounds really cool!
The only down side I can see is that it would be possible to add malicious code that gets executed for unsuspecting users without proper review steps.

Only allowing branches from old contributors would make this less of a problem, but still a potential threat.

How about if we added a time limit as well so it only adds branches that’s been in review for x-days? That way there would be a window to find malicious code.
Post Reply