I think the focus should be on creating a set of practices that creates easy to review PRs. These PRs should result not in a 'Looks good to me' comment, but a 'I understand this PR and see no issues in merging'. The latter requires understanding the changes, which has been a challenge for me (and I think others, but not sure). For example, looking at one of the CSS refactors w/o having worked on the frontend in a while, it's rather hard to really see if there are any issues.

Talking w/ @mnapoli about the testing environment/DI refactor I did before leads me to believe we could create a set of practices that will result in PRs that are easy to review. For example, commit messages that are descriptive about why changes are there. Using interactive rebases to reduce clutter (so reviews can be done commit by commit), etc.

+1 for avoiding big pull requests, and trying to have one commit = one logical change. Rebasing (i.e. reordering + squashing + renaming commits) is awesome, it takes some time to get used to it (still learning) but I've come to love it. Sometimes a bigger pull request is not so much an issue when there are few and very explicit commits (we can review commit by commit, just like we would review small pull request by small pull request).

I also think some changes shouldn't be pull requests, i.e. things like small bugfixes should be committed straight to master. Maybe we'll find the optimal balance between commit-to-master/pull-request over time, I think right now we have gone a little too far on the pull request side. Do you share the same feeling?

Finally I have a big problem with the builds: all the pull requests I see are red (including mine). This makes opening them and reviewing slower and harder (in theory a red pull request shouldn't even be up for review). E.g. I end up rebasing pull requests all the time, e.g. to rebase on master when it's finally green.

• we need to keep the build green (which is hard I know, the next point should help)
• we need to solve the problem of UI tests in pull requests (with git-lfs Move UI screenshots to git-lfs #7726 I want to push that ASAP) so that we can update screenshots and have green UI tests in PRs

Once we have that at least we can say: "if master is green, we don't merge a red pull request". Ideally it should be "we don't merge a red pull request" but we are too far from it yet since a PR based on a red master cannot be green ;)

The latter requires understanding the changes, which has been a challenge for me (and I think others, but not sure).

Yes this is a challenge for me too on many pull requests (many parts of Piwik I still don't know well). I've tried lately to spend time on learning parts I don't know, but it takes time… Committing smaller changes straight to master would limit that though.

I also think some changes shouldn't be pull requests, i.e. things like small bugfixes should be committed straight to master. Maybe we'll find the optimal balance between commit-to-master/pull-request over time, I think right now we have gone a little too far on the pull request side. Do you share the same feeling?

Personally, it's hard to know what other developers consider minor/needing review, namely because I'm not inside their heads :).

Also, +1 for git-lfs

Yes this is a challenge for me too on many pull requests (many parts of Piwik I still don't know well). I've tried lately to spend time on learning parts I don't know, but it takes time… Committing smaller changes straight to master would limit that though.

I'm not sure how not reviewing smaller changes would be helpful here? For me the issue has been commit messages that aren't very specific or helpful. Comments by the author in the PR can be helpful, but usually they are not about things I am worried about, and just get in the way of looking at the code.

I'm not sure how not reviewing smaller changes would be helpful here?

My point is exactly that we wouldn't have to review them ;) Some small changes are reviewed (i.e. take time away) just because that's how we do. IMO pull requests are most useful to review architectural changes (and also learn about them), or critical changes like stuff related to public API, etc. Small changes are OK if could be implemented better (because small changes are easy to change later). Big changes and API changes have a much higher cost to refactor, we want to avoid going back and forth on this.

I would not issue a pull request for everything either, especially not for "small" changes unless it effects public API in any way (Plugin/Theme API + HTTP API + database). It would be up to everyone whether one thinks it is a "small" change or whether it needs review or not.

so reviews can be done commit by commit

I never do reviews commit by commit. Not really helpful for me and not interested in it. So for me it's not important and I won't merge/rebase them into multiple commits that logically make sense as I don't work like that. Rebasing into one commit is doable (for me) though. I think for a good review it's not really helpful at just looking at the diff anyway (at least not for me personally as it is hard to see any side effects etc in just a diff, not saying only looking at a diff is a bad review). One will have to check out the branch anyway.

What are challenges we're facing and how could we improve?

In previous projects we basically always had a look at "needs review PRs" before starting any new issue. Once there was no issue left that needs review, one was "allowed" to start working on a new issue. It shouldn't be a hard rule as one maybe can't review all PRs as one just has not the knowledge about everything.

If i can give my opinion...

• Nothing should EVER be merged directly in master. All code should be reviewed. Developers are humans and humans make mistakes. Let people merge directly into the master means you're allowing some unexpected or unknown mistakes to reach the source. There is no reason for not reviewing something except laziness.
• Nobody should merge its own PR.
• PR's should NEVER be merged with red test status. If the tests are failing, fix them before merging the PR. If you don't follow the "always review rule", you'll get some red status on master at some point, inevitably. This fix should preferably be done in another PR, unless the cause of the fail is related to what is being modified in the current one.

That said,

• PR's should be as simple as possible to address only one feature or bug. The number of commits isn't a issue. Commits represents workflow, and workflow should be in history. If you modified something, and reverted it, its ok because that's part of the workflow. Unless the developer is modifying one letter in each commit.
• PR's could, but should not be reviewed by commits. What is being requested, is to merge the result of many commits, not each single one. If something got modified, and later this modification was removed or modified again, it makes no sense to force the reviewer to understand it, because it will not be part of the source.

Question:
Is it really necessary to run SystemTests, UnitTests and IntegrationTests in separate build jobs? From the few I studied about the settings, and I might be wrong, these tests are repeated in AllTests. Aren't they? If that is right, by removing these jobs, it would save 25 minutes in the total time. Fast tests result in fast reviews/merges.

Nothing should EVER be merged directly in master...

... There is no reason for not reviewing something except laziness.

"Ever" is a big word. While it is correct what you mentioned, I think we do sometimes have to make trade-offs as our time is limited and small changes can be acceptable to directly push to master. Eg when it is a change with very limited risk, when it is covered by good tests, or simply when the developer is very sure it'll work and thinks it is ok to skip the review process in that case (this works as only a few developers can push directly and we trust those devevelopers to make such decisions as we're all aware about the benefits of a reviews and the risk when we skip this process ).... I believe a commit to master gets usually noticed anyway as we get notified via email. Meaning someone would most likely still have a look and it will be "reviewed" anyway but we skip the process for it.

Agree with the other things.

Re that question: AllTests runs the same tests but with Mysqli instead of PDO therefore needed. In theory we would not have to execute the UnitTest in AllTests but think it takes only 30 seconds or so anyway.

I think we do sometimes have to make trade-offs as our time is limited and small changes can be acceptable to directly push to master

Small changes are also easy to review, taking few time.

In the big framework projects out there, like ZF2 or Symfony, even ppl with write permission never push directly to master. There is a development branch (unstable version) so the code is tested for months even after a merge. Code is always reviewed even if you're updating a docblock code style and I think this is a good practice.

Even the best developers make mistakes. A single missing comma may break the entire application.

Even the best developers make mistakes. A single missing comma may break the entire application.

Exactly, there is no guarantee that having code reviewed will make it bug-less (if one developer can miss a bug, then 2 or 3 could too). Code review should not be relied upon to detect bugs, that's the role of automated tests (which are far less expensive in that regard).

IMO code reviews are mostly useful to discuss anything that has a significant impact on other developers and the project:

• big features, new plugins
• architectural changes
• changes to core components/important parts of Piwik
• large codestyle changes (to give an example of something that has a very "soft" impact)
• ask question/feedback on something in particular

For e.g. typo fixes or small bugfixes the cost of changing that later is minimal so I don't think it's worth blocking for days and take time off other people for reviewing.

For e.g. typo fixes or small bugfixes the cost of changing that later is minimal so I don't think it's worth blocking for days and take time off other people for reviewing.

Of course blocking for days to review a simple typo or small bugfix is a big problem. But I think you guys should focus on how to make faster reviews rather than skipping the step.

Doing simple changes direct to master will not be a big speed improvement since the reviews for that kind of change would also be very fast (or at least they should).

@mnapoli The suggestion you and @diosmosis gave above, to make the PR's simple is the best way to reach that IMHO. I understand how boring is to review a PR with 10+ files in the diff. If you have more PR's with simple changes everybody gets more interested in reviewing, which will turn into faster merges.

Also there are more two problems. The current coverage value at 15%, does not offer enough reliability even on green reports. You have to trust developer ran the tests, and if he didn't you'll have a failing build. Pull-request is the only way to be sure everything was tested before the code reaches the master branch.

if one developer can miss a bug, then 2 or 3 could too

You're right. Its more a matter of making sure that change is valid than detecting bugs. Sometimes the developers will also try to do things they shouldn't be doing. Maybe It takes less time to review a small change and make sure its valid than debugging and reverting it later.

Great discussion & ideas.

We agree that most changes are done in pull requests but this is not hard rule... Sometimes it is acceptable to commit small/trivial changes to master directly. so far IMO we have striked a good balance between creating PR and merging to master small changes.

Stat: last month about 75 pull requests were opened. That's about 1 or 2 pull requests per day to merge, if we count 3 to 4 full time core team members. -> How do we organise our day so that we review one to two pull requests each day?

We want to make reviewing and merging pull requests easier:

• we will aim to keep pull requests short: they are easier to review and are merged faster,
• pull requests with unit tests and maybe other tests (such as screenshot tests) are easier to merge,
• we try to create PRs with a useful commit history or squash commits when the history is not useful,
• we need consistent green builds for master and Pull Requests 💚
  • git-lfs will help us removing awkward submodule (Move UI screenshots to git-lfs #7726)
  • we will also need to fix remaining UI build random failures (Some UI screenshot tests fail at random  #6693)

What are challenges we're facing and how could we improve?
In previous projects we basically always had a look at "needs review PRs" before starting any new issue. Once there was no issue left that needs review, one was "allowed" to start working on a new issue. It shouldn't be a hard rule as one maybe can't review all PRs as one just has not the knowledge about everything.

as you say we don't want to set a hard rule, but each of us could try and practise this each day to some degree. Reviewing other PR will help all of us keep our WIP stack small and minimise context switching over time (making us healthier and happier!).

Message to: @tsteur @diosmosis @mnapoli @sgiehl and others reading this: can we try to review at least one team mate's pull requests as part of our daily morning routine? 👍

happy to continue discussion here if there is any more feedback or idea!

To merge PR's faster and to having to switch less between tasks it might be also worth thinking about what to actually "review" in PRs. I think we had this kinda topic already that we want to eg focus on Security, Performance, etc. I think minor "issues/things" such as an if could be merged with another if could be just ignored IMO and a PR having only "minor" issues could be just merged directly.

I don't wanna open a discussion on what to review here. Everyone maybe considers different things as important. Just saying some minor things are maybe not worth waiting merging a PR

One thing that we might also differentiate in PRs are things that are only internal and things that affect the public API in any way. For example PHP API's, HTTP API's, translation keys, less variables, ...

Things that affect the public API are way more important to review and discuss as we cannot change them easily and they affect many users. Here we should focus in PR's and maybe sometimes even require two 👍 . Whereas in internal reviews we could try to merge them faster by being a bit less "strict" with things. Eg I often just check whether there are tests, re Security, try to identify easily spotted bugs etc. Everything else can be changed later if it turns out to be not as good or if requirements change.

Sometimes, when the pull request is risky, security related, can potentially screw data, etc. we must do a thorough review. For example atm i'm reviewing thoroughly #7887. As part of this review I check out the branch, and test. Often we have minor feedback such as: "rename the parameter --site to --idsite" or "rename the command to XYZ". Sometimes, when we have such minor feedback, it's more time efficient and useful if the reviewer directly makes the changes on the author branch. In this case, i'm directly making those trivial changes so that Benaka does not have to again go back into this code and make all these changes, then wait for tests, etc. It saves the PR author some context switching and time wasted to wait for tests etc.

TLDR: as the pull request reviewer, it is sometimes more beneficial to team happiness if reviewer directly commits to the branch and makes the trivial changes himself.

Things that affect the public API are way more important to review and discuss as we cannot change them easily and they affect many users.

fyi public APIs are described in #8125

Before closing this issue, let's create a small document about "Creating Pull requests: best practises" that summarizes our discussion and useful ideas in this thread. Could one of you work on creating this small doc?

Maybe something like this?

Creating Pull requests: best practises

• We try to avoid big pull requests and aim for small PRs that are easier to review
• When issuing a PR we set a label Pull Request WIP and replace this label with Needs Review once the PR is done. If a PR refs another issue we assign the label not-in-changelog
• A PR should contain a description explaining things if useful. It should contain as much as necessary and as less as possible.
• Small changes can be merged directly without a review if the developer is 100% certain the change won't have any side effects etc. It is always recommended to still quickly ask another developer that is online to have a look at this PR now as such PRs are quickly reviewed. If a PR affects the public API in any way a PR should not be merged without a review
• Before working on a new issue it is recommended to check for pending PRs that have a Needs Review label
• When reviewing a PR it is important to check things like Security, Performance, etc. Minor "issues/feedback" such as an if could be merged with another are less important. If a reviewer notices only such minor things, we can merge the PR directly or the reviewer can make the changes directly and merge afterwards.
• PRs that affect the public API or security need a thorough review. For other PRs it is always good to keep in mind that we can change later at anytime. Things therefore don't have to be "perfect" as long as the formal requirements are given (eg an entry in the developer changelog if needed)
• Pull requests should contain tests

Small changes can be merged directly without a review if the developer is certain the change won't have any side effects etc. If a PR affects the public API in any way a PR should not be merged without a review

This one is a little bit tricky. Of course there are obvious things like small UI tweaks with updated screenshots but there are also small changes that may break things (and dev may be confident that it's a minor change even though it isn't). TBH I would review all the code except docs/readme update.

I know it's a little tricky. We kinda had this discussion in the other comments. If a developer is 100% sure that it doesn't affect anything then it can be merged directly. This includes docs/readme updates, some minor UI changes, things with very limited scope etc. If a developer doesn't know much about Piwik or in which way it works or what it can affect then this PR shouldn't be merged directly.

As a fallback we always have tests of plugins (we won't notice anyway whether it will actually break any of our plugins until it's in master and all tests of all repositories are restarted). I know they don't cover everything. Also we often get emails for commits / merges anyway and there's still a chance that one has a look at a PR/commit anyway.

I'd just like to keep this bureaucracy as small as possible. Strict rules are not really helpful I think. We could maybe change it too:

Small changes can be merged directly without a review if the developer is 100% certain the change won't have any side effects etc. It is always recommended to still quickly ask another developer that is online to have a look at this PR now as such PRs are quickly reviewed. If a PR affects the public API in any way a PR should not be merged without a review

assigning to @mattab , don't know whether summary is ok and into which "document" this goes

All developers and future contributors can now enjoy our Creating Pull requests: best practises guide.

thanks @tsteur for the text.