Quality Assurance measures

[hmmmm]

I think we ought to reconsider some of our quality assurance measures.

Code Reviews
Two other core developers are supposed to review a commit before it gets pushed to upstream. Developers in charge of a specific subsystem get a free pass on their own commits to that subsystem.
Why this sucks:
Has anybody actually tried getting two core developers to review your commit? It's very difficult.
1). Developers are geographically distributed. They live in very different time zones.
2). Developers usually have a narrow time window to work on Minetest-related tasks. It's a side project, not their day job. This aggravates issue #1.
3). Waiting for feedback contributes to code rot. By the time it gets to actually be merged, several merge conflicts crop up.
4). Code reviews are usually superficial. I've noticed that reviewers often miss big, obvious flaws with commits. Reviewers don't test the code they review. Even then, the subject of the manner in which the code fixed/added something takes a back seat to less pertinent details such as code style.

Coincidentally, Minetest uses the same exact code review setup as my team does for my day job, but they key difference is that our upstream commits go directly into production and we're not in a constant rush to develop. Minetest has releases and generally moves faster. I pine for something better that is not a hindrance to development. Core developers, at least, presumably aren't running around destroying the codebase as they are considered responsible. They sure do make stupid mistakes sometimes, even in their own corner of the code world. I've done it and three developers presumably reviewed my code and missed the obvious mistake. I would be all for code reviewing if it were actually catching concerns that aren't minor code style issues or exercises of coding ideology. The code reviews are more or less acknowledgement of what somebody intends to commit before they get waved on to proceed with pushing to upstream - I guess how prevalent of an issue this is varies on the quality and skill of the developer performing the review.

Unit Testing
Unit testing is great to have. Runtime errors could be caught faster and with less effort by automating the verification of any changes.
Currently, no new unit tests seem to be added and existing ones are only modified if necessary due to a functionality change.
At the bare minimum, we could somehow automatically deny pushing a commit if it causes any unit test to fail. Bogus test failures would necessitate fixing the tests so they pass. Even better would be to encourage writing unit tests for as many things as possible, and perhaps deny code from being pushed upstream if it lacks unit tests that can reasonably be implemented. The code review model might be a roadblock to this latter proposal.

Much of the code is currently written with an interface lacking separation of concerns not conducive to unit testing when most of the codebase does not actually need to be this way. We want to draw a fine line between reasonable interfaces and test-driven design. The latter, in cases of features that require a lot of state, make testing feel forced and take same amount of the code as the feature itself - and then some things are impossible to test due to the nature of the application.

I need help coming up with a reasonable policy for adding unit tests. I can't force anybody to create them since for much of the code it's rather difficult to automate, and forcing people to do something would only result in the bare minimum. Just see how much handwavey the code review process became for evidence.

[rubenwardy]

Unit tests are good, but there some things that can't be fully tested with this, like graphics and shaders.

How about requiring the pull requester to supply a list of manual tests they did to show that it works, and other people can expand on. Small changes like code style and comments are exempt.

You recently directly commited a change to upstream which hid warnings. This also hid some bugs, and should of been done in your cmake/gcc installation if it annoyed you, there are settings to hide it you can change without changing Minetest. Just saying. So there should be people checking. But that can't stop progress. All pull requests need to be gone through and talke about, merged, labelled as wont add, or closed.

[PilzAdam]

Our current guidelines to push upstream are that 2 core devs need to agree to merge a pull request. It does not say that they need to review the code.

In practice I often observed that one core devs "takes care" for a PR (i.e. reviewing the code (and possibly fixing small issues themselves), testing, etc.) and the other core dev just generally agrees to the feature that is added by the PR. This is a good system IMO, since otherwise core devs might add features that are not wanted by anyone else (and reverting a feature is a lot more work than blocking the PR in the first place).
The current situation with one core dev "taking care" of a PR can be strengthened by using the "assigning" mechanism of Github's issue / PR list. Core devs can assign themselves to PRs, to show others (and possibly remind themselves) that they will take care that it will be merged in a reasonable time. Getting another core dev to just quickly look at the feature is a lot easier than getting someone to review the code.

I generally like that we have a rather stable development branch, and that we have quite a lot of "testers" that use the latest dev version regularly. We should see this as a benefit.

As for merge conflicts: I only see merge conflicts very rarely, so this is not a huge issue IMO.

[Calinou]

As for merge conflicts: I only see merge conflicts very rarely, so this is not a huge issue IMO.

Well, stuff like this is very sad to see.

[hmmmm]

You recently directly commited a change to upstream which hid warnings. This also hid some bugs, and should of been done in your cmake/gcc installation if it annoyed you, there are settings to hide it you can change without changing Minetest.

I'd just like to point out this is false. The warning in question would need to be explicitly enabled for everybody using newer versions of gcc, so in effect nothing was being hidden at all. Nobody even knew those warnings existed until I made that commit. The objection to my patch was purely ideological in nature, it made no real impact on development.
Besides, what about -Wno-unused-but-set-variable? Why don't I see a mass of outrage over this?

In practice I often observed that one core devs "takes care" for a PR (i.e. reviewing the code (and possibly fixing small issues themselves), testing, etc.) and the other core dev just generally agrees to the feature that is added by the PR. This is a good system IMO, since otherwise core devs might add features that are not wanted by anyone else (and reverting a feature is a lot more work than blocking the PR in the first place).

WTF PilzAdam, releasing and now this? Our development model is far from roses and sunshine. If you don't reform, you'll lose the serious developers because they won't want to spend time and effort on a joke of a mismanaged project.

That's not a good system. The whole reasoning behind two eyes is to make sure upstream stays kind of stable by not introducing obvious bugs, which like I said only sort-of works depending on who's active in the channel at the time.
Example: if I weren't in the channel at the time RBA was asking to push this, some others more braindead than I would have given the ol' handwave and let him introduce a rather obvious showstopping bug into upstream which to fix requires...

• disappointing our users
• expending effort on tracking down the bug
• fixing it
• making another PR for the commit that fixes it
• wait for more approval and then finally fix the original issue.

If you want to veto high-level features instead of verify functionality, then maybe we could have discussions of each developer's personal roadmap before we begin work on it. Code reviews are not for this purpose.

Also I am having trouble seeing what's so hard about reverting a change: "git revert <hash> && git push upstream master". Can you expand on that?

Merge conflicts happen all the time and are quite messy, to the point where we have to schedule who's working on what files to avoid conflcits.

[PilzAdam]

You recently directly commited a change to upstream which hid warnings. This also hid some bugs, and should of been done in your cmake/gcc installation if it annoyed you, there are settings to hide it you can change without changing Minetest.

I'd just like to point out this is false. The warning in question would need to be explicitly enabled for everybody using newer versions of gcc, so in effect nothing was being hidden at all. Nobody even knew those warnings existed until I made that commit. The objection to my patch was purely ideological in nature, it made no real impact on development.
Besides, what about -Wno-unused-but-set-variable? Why don't I see a mass of outrage over this?

In practice I often observed that one core devs "takes care" for a PR (i.e. reviewing the code (and possibly fixing small issues themselves), testing, etc.) and the other core dev just generally agrees to the feature that is added by the PR. This is a good system IMO, since otherwise core devs might add features that are not wanted by anyone else (and reverting a feature is a lot more work than blocking the PR in the first place).

WTF PilzAdam, releasing and now this? Our development model is far from roses and sunshine. If you don't reform, you'll lose the serious developers because they won't want to spend time and effort on a joke of a mismanaged project.

That's not a good system. The whole reasoning behind two eyes is to make sure upstream stays kind of stable by not introducing obvious bugs, which like I said only sort-of works depending on who's active in the channel at the time.
Example: if I weren't in the channel at the time RBA was asking to push this, some others more braindead than I would have given the ol' handwave and let him introduce a rather obvious showstopping bug into upstream which to fix requires...

• disappointing our users
• expending effort on tracking down the bug
• fixing it
• making another PR for the commit that fixes it
• wait for more approval and then finally fix the original issue.

If you want to veto high-level features instead of verify functionality, then maybe we could have discussions of each developer's personal roadmap before we begin work on it. Code reviews are not for this purpose.

Also I am having trouble seeing what's so hard about reverting a change: "git revert <hash> && git push upstream master". Can you expand on that?

Merge conflicts happen all the time and are quite messy, to the point where we have to schedule who's working on what files to avoid conflcits.

I don't understand you anymore. First you say that approving pull requests without looking closely at the code is bad since it might introduce bugs, and in the next sentence you say that reverting is easy and you imply that the dev branch doesn't need to be as stable as it currently is.

By "reverting is a lot more work" I meant that we currently have no fixed guidelines how to revert things. The current practice is that we talk to the dev who pushed the commit and try to convince them that reverting the commit is the right thing to. Try convincing somebody to revert a commit that adds a feature nobody else wants...
(also it's basic psychology that it's harder to take something away from people onces they had it than not giving them the thing in the first place. So you will always get people complaining if features are removed, even if they are not good for the project.)

[hmmmm]

I don't understand you anymore. First you say that approving pull requests without looking closely at the code is bad since it might introduce bugs, and in the next sentence you say that reverting is easy and you imply that the dev branch doesn't need to be as stable as it currently is.

No, the idea I'm trying to get across is that "code review" as it exists right now is pretty worthless. Either fix it and increase the stability of upstream, or stop whining about it altogether. It's a mere hindrance to development because it acts like a road block but it doesn't add any value so the hindrance isn't justified.

I brought up reverting since you mentioned it was too difficult to be the solution for unwanted "features" added by surprise.

By "reverting is a lot more work" I meant that we currently have no fixed guidelines how to revert things.

That makes more sense. It's not difficult, we just need to create guidelines.

[PilzAdam]

I don't understand you anymore. First you say that approving pull requests without looking closely at the code is bad since it might introduce bugs, and in the next sentence you say that reverting is easy and you imply that the dev branch doesn't need to be as stable as it currently is.

No, the idea I'm trying to get across is that "code review" as it exists right now is pretty worthless. Either fix it and increase the stability of upstream, or stop whining about it altogether. It's a mere hindrance to development because it acts like a road block but it doesn't add any value so the hindrance isn't justified.

I brought up reverting since you mentioned it was too difficult to be the solution for unwanted "features" added by surprise.

Ah, okay, that makes more sense. I see your point that you think that the current model hinders development too much.
My suggestion would be to remove the code review, but keep the "2 devs need to agree" rule for features, like we currently have. This way we don't hinder development and still can prevent unwanted features. I would also encourage that the people who want to add the features talk to devs before they start coding it, to prevent unnecessary work for them (i.e. core devs can approve features before they are coded).
What a "feature" is needs to be discussed, to make it clearer to devs what is allowed to be pushed immediately and what needs approval from others.

By "reverting is a lot more work" I meant that we currently have no fixed guidelines how to revert things.

That makes more sense. It's not difficult, we just need to create guidelines.

Yes, we need guidelines for this. Any suggestions?

[hmmmm]

My suggestion would be to remove the code review, but keep the "2 devs need to agree" rule for features, like we currently have. I would also encourage that the people who want to add the features talk to devs before they start coding it, to prevent unnecessary work for them (i.e. core devs can approve features before they are coded).

I like this and I announced my personal roadmap a few days ago in the IRC channel. We should probably formalize this process.
I still think code reviews are in theory a good thing and we should be doing them, but only if more developers put real effort into code reviewing. Unfortunately I don't believe this is likely to change because developers will only do what they feel like doing (coding) since they're not being paid for it. Coding is really fun, but the message we should try getting across is that reviewing code, testing, writing tests, etc. is helping Minetest get better, and helping Minetest get better is lots of fun too.

What a "feature" is needs to be discussed, to make it clearer to devs what is allowed to be pushed immediately and what needs approval from others.

I suggest we call things features that obviously add functionality that didn't exist before. This is an overly simplistic definition that correctly classifies most features. 90% of the time you can tell what a feature is prima facie; changes that aren't as obvious should require approval anyway, even if it's slightly ambiguous, to be safe.

Yes, we need guidelines for this. Any suggestions?

Maybe like 3 or 4 core developers have to agree on reverting a change and for a damn good, documented reason, like causing a demonstrable problem that cannot be trivially fixed with a follow-up commit.

[jp]

The hmmmm's roadmap, for recall : http://fpaste.org/164229/14199135/