查看原文
其他

code review的几条规则

Elena Flat 高可用架构 2020-11-06

规则1


每个 PR 审查至少需要 2 个同组开发者的批准,管理者的审批不统计。


首先要注意的是,由于我所在的是一个 3 人团队,这是最理想的。所有的 修改 3 个开发者都 100% 知情。如果团队规模更大,情况可能会有所不同。你所追求的是邓布利多和死亡圣器(Dumbledore Horcruxes)的情况,如果你死了,至少有 2 - 3 人知道死亡圣器的事。我们的团队现在有 5 个开发者,2 个老手,1 个新人,2 人中等水平。我们仍在努力坚持每个人都要审核每个人 PR 的模式,但至少 1 个老手 + 1 个其他开发者的模式,将来可能更可行一点。


管理者审批为什么不算数?你也许认为“我是经理,也是个了不起的程序员,我的审核完全算数”,经理们的 review 确实有帮助,有大把管理者也是优秀的程序员,当他们有时间 code review 时,他们往往会有很好的建议或想法来帮助其他开发者。


但你必须考虑管理者花在编码上的时间有多少?如果超过 50%,那么他们也可以算作开发人员,这种情况作为审核者也是可以的。但是在我们公司,经理只是偶尔做一些非常偶然的编码工作,因此,主力还是要靠开发人员自己来说话。我不希望看到经理和开发人员之间的无限审批周期,一个管理者不断地审批一个个开发人员的PR,即使团队里还有 3 个开发人员有大把时间可以来做这个工作。


规则2


每个 PR 必须有一个好的描述,通过阅读描述,审核者能够理解代码的目的是什么。即使有一个 Jira ticket 或需求页面,也必须满足此要求。


没有描述的 PR —— 在我的团队中,这种情况永远不会通过。最好的情况是:他们没有写,是因为有另外一个 Jira ticket 页面有很好的描述。最坏的情况是什么也没有。


遇到没有描述的 PR,审核员有 2 个工作:


  1. 通过阅读 PR 中的代码修改,了解代码的作用。

  2. 审阅者试图夜观天象来判断代码是否做了它应该做的事情?


如果没有清楚地说明代码要做什么,就不可能审查它的正确性。你根本无法知道什么是正确的,你的操作是基于你认为正确的假设。


规则3


PR 必须有足够的单元测试和集成测试覆盖。


理想情况下,审阅者可以很容易地看到测试用例列表。


如果测试覆盖率已经存在,或者由于某种原因测试覆盖率不完整(工作被拆分到另一个依赖的 ticket),PR 描述中应该提到这一点。


给出好的 PR 评审意见很难、也很费时间。这是因为一旦你明白了代码应该做什么,你就必须停止审查并去查看你想要的测试覆盖率。你要合适的测试用例列表,并决定是哪种类型的测试:单元测试/集成测试/端到端。


你还必须考虑是否有可能对生产环境产生影响,以及是否需要在部署前对真实数据进行测试。


现在你知道了这些,你再去回顾一下 PR,看看他们是否涵盖了你所想的一切。理想情况下,这是一个混合体:你想到了他们没有想到的东西,他们也想到了你没有想到的东西,然后你们一起做得很好。这就是能够很容易看到 PR 中涵盖的测试用例列表的地方(相对于阅读 100 行或 1000 行的测试,必须弄清楚哪些用例是真正被测试的)。


注意,如果遵循了规则 2,你可以通过阅读描述就知道代码应该做什么。所以你可以在审查任何实际的代码之前,就可以完成审查测试覆盖率的步骤!


我个人喜欢把所有这些东西:相关的测试,真实数据测试,对线上客户端影响,生产环境配置更改都放在 PR 描述中。或者其他合适的地方,在一个地方统一查看。


规则4:

如果 PR 是一个 bug 修复,它必须包含一个测试,如果这个 bug 被还原,这个测试就会报错。


我以为这很明显,但就像大多数明显的事情一样,事情常常不是这样。


假设有一个打字错误的 bug,你用  != 代替了 ==,当时是晚上10点,你不应该去写代码,但你觉得那天晚上你想多做一些事情。于是你打开了一个 BUG 修复 PR,把 != 改成了 ==,有一个好心人也在工作,批准了它,然后把它送到了主分支。


三天后,你的好友 Duke 的 PR 也终于准备好了,他以为这是一个小功能,没有把它拆成小的 ticket,但事实并非如此,现在他有一个 2000 行的PR,他恳求团队进行代码审查。


Duke 执行了代码合并,代码冲突的地方你把 == 换成了 !=,Duke 有些疲惫,他没注意到这些细节,因此 != 又回来了。


对,因为没有测试来捕捉它,bug 就又回来了。


这是个虚构的例子,但如果你长期观察,这种情况经常发生。把团队的 bug 铺开 3 年,你会看到那些没有被测试捕获的 bug 又出现了。


总结


对,就这些! 实际只有 3 个规则,第 4 个更像是常识性的东西。


这些规则让我们的团队产生了一些质量相当不错的代码。对此,我很高兴,但也绝对不是我个人的功劳。


同样的,你也许认为这些规则对你也有帮助,但也许不太认同这些规则。不管怎样,如果这些规则引发了你的思考或想法,我很高兴。


原文:

https://medium.com/inside-league/how-one-code-review-rule-turned-my-team-into-a-dream-team-fdb172799d11


参考阅读:


本文由高可用架构翻译,技术原创及架构实践文章,欢迎通过公众号菜单「联系我们」进行投稿。

高可用架构
改变互联网的构建方式

长按二维码 关注「高可用架构」公众号

    您可能也对以下帖子感兴趣

    文章有问题?点此查看未经处理的缓存