Helping to review pull requests

[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 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]

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.

[voskos]

May I suggest a industry accepted way to speed up merges is more automated testing.

[chrisb]

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.

[voskos]

Its 00:30 what doe

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 I am trying here, give me a break
Boost unit tests would be nice to have also

[chrisb]

Its 00:30 here and I trying to count edges to see if refine=true did anything

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.

[hyarion]

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.

[watsug]

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]

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]

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.