我们是怎么做Code Review的

有些经验应该可以和大家一起分享、探讨。我们为什么要推行Code Review呢我们当时面临着代码混乱、Bug频出的状况。当时我觉得要有所改变希望能提高产品的代码质量改善开发团队面临的困境。并且我个人在开发上有很多经验也希望这些知识能够在团队内传播。各种考虑后我们最后认为推行Code Review能改善或解决我们面临的很多问题。这篇文章的目的不是告诉大家怎么在一个团队内推行Code Review首先因为我个人仅在一家公司内推行过并没有很多经验。其次每家公司、每个团队的情况都不太一样应该根据公司或团队的实际情况选择恰当的方案并根据成员的反馈来及时调整推动Code Review的实施。所以本文是介绍我们公司是如何实施Code Review的我们是如何解决我们遇到的问题的希望我们的经验能给大家带来些帮助。行文仓促如有遗漏或错误欢迎指正。一、流程和规则经过简单的对比、试用我们最后采用了Git FlowPull RequestPR模式来做Code Review。PR模式详情可参见 Git工作流指南Pull Request工作流Pull Request(PR)简单的说就是你没有权限往一个特定的仓库或分支提交代码你请求有权限的人把你提交的代码从你的仓库或分支合并到指定的仓库或分支。由于PR需要有权限的人确认所以非常适合在这个过程中做Code Review是否接受或者拒绝就取决于Code Review的结果。在支持PR模式的软件里每一个PR都有一个新增代码的对比diff界面。代码审核者可以在线浏览请求合并的新增代码并针对有疑问的代码行添加评论通过这种方式来实现Code Review。评论可以被所有有权限查看仓库的人看到每个人都可以回复任何人的评论有点像论坛里某个帖子的讨论。这种模式是事后审核也就是代码已经提交到了中心仓库Review过程中频繁的改动会造成历史签入记录的混乱。当然Git可以采用更改历史记录来解决这个问题由于容易误操作我们一般只在基础类库这类要求比较严格的项目上实施。我们所了解到的支持PR模式的软件都采用Git作为源代码版本控制工具所以我们的源代码版本控制工具也迁移到了Git。由于Git太灵活了因此诞生了很多的Git流程用来规范Git的使用。常见的有集中式工作流、功能分支工作流、Gitflow工作流、Forking工作流、Pull Request工作流。我们对Git Flow做了些调整调整后的流程被命名为Baza Flow定义见后文。根据Baza Flow我们大部分仓库只定义了2个主干分支master和develop。(例外我们有一个仓库有3个开发小组同时进行开发定义了4个主干分支目前还比较顺畅再多估计主干分支之间的合并就比较繁琐了。)master对应生产环境代码所有面向生产环境的发布来源都是master分支的代码。develop则对应本地测试环境的代码。绝大多数情况下QA测试只测试develop分支和master分支的代码。由于开发人员都在一个团队内所以我们没有采用基于仓库的PR采用的是基于分支的PR。我们对主干分支的操作权限做了限制只有特定的人才能操作develop分支是项目开发Leader和架构师master分支是QA。有权限往主干分支合并的成员会按照约定的规则来执行合并不会合并没有完成审核的PR。上面这点其实蛮重要的所以我们会对有权限合并的人有特别的约定在什么情况下才能合并代码。见后文PR的说明PR的发起人要主动的推动PR的审核Leader也会密切关注PR审核的进度在需要的时候及时介入。我们配置了CI服务器什么是CI只编译特定的分支通常是develop和master分支。所有的代码合并到了主干分支之后都会自动触发编译和本地测试环境的发布QA无需依赖开发人员编译的代码来测试也无需自己手工操作这些保证了开发人员和测试人员的相互独立。我们本地测试环境的发布包含了数据库和站点的发布全自动的发布完成以后就是一个可用的产品有时间这部分也可以分享一下。我们还使用了Scrum里面一个很重要的概念完成定义。就是我们规定了我们一个任务的完成被定义为代码编写完成经过自测提交的PR经过审核并且合并到主干分支。也就是说所有的代码被合并到了主干分支之后任务才算是完成而被合并到主干分支必须要经过Code Review这是强制的。Baza Flow当前版本 V0.9Baza Flow 由 Git Flow 演化而来Git Flow的开发模式如下图所示由于我们的托管软件对于Pull Request的限制我们对Git Flow做了改动改动的地方有1、每一个大功能我们会创建一个单独的feature分支项目开发人员基于这个单独的feature分支创建自己的任务分支。比如对于CS 2项目来说启动的时候分支的创建是master - develop - feature/v2。开发人员应该基于这个大特性分支feature/v2来创建自己的任务分支比如创建XXXX可以用一个单独的分支feature/v2-xxxx。完成这个任务以后立即向上游分支feature/v2提交pull request。然后从feature/v2-xxxx 创建自己的下一个任务分支比如YYYY编辑 feature/v2-yyyy。请注意合并到上游分支的功能必须相对独立而且是可用的分支任务工作量0.5-1个工作日不宜超过2个工作日超过2个工作日不向上游合并需要向团队解释。代码经过Review以后可能会进行必要的修改修改在原分支修改修改完毕代码合并进上游分支原分支会定期删除。项目组成员在收到合并成功的通知后请自行从上游大特性分支向下合并到自己当前的开发分支。提交pull request后创建新任务分支的时候务必知会一下相关配合同事比如前端的同事让他们在新的分支上继续开发。2、对于小功能预计在0.5-1个不超过2个工作日工作量的开发任务直接基于develop分支创建特性分支即可。3、在各个分支遇到的bug请基于该分支创建一个Bug分支。如果在缺陷跟踪管理系统上有对应的项命名请使用缺陷跟踪管理系统的ID比如BAZABUG-1354 比如这个Bug的分支命名就是bugfix/BAZABUG-1354。如果在缺陷跟踪管理系统上没有对应的项命名请简短的说明修改内容比如“JX 9df2b01 引用bootstrap css虚拟路径重写避免出现字体无法找到的问题”分支命名可以是bugfix/miss-font。完成修改以后提交并推送到中心仓库然后立即向上游分支提交pull request。4、发起pull request以后请将pull request的链接在IM上发给代码审核者以此通知对方及时进行审核。二、执行我们在团队内部提倡质量优先开发团队不能为了进度牺牲质量并在团队内部达成了共识。所以无论进度有多么紧迫Code Review的过程都一定会做。所有的问题一定会被提出只是会根据进度的紧迫程度以及问题的大小改动成本决定问题是现在解决还是加一个TODO并记录在缺陷跟踪管理系统内以防日后遗忘。多数情况下我们都会要求立即解决哪怕因此造成了发布的推迟。我们深知其实多数情况下现在不解决日后不知道猴年马月才能解决。我们在团队内推行Code Review的过程中没有遇到太多阻力。原因大概有两点首先管理层方面了解之前遇到的各种问题也迫切希望能有所改善所以从一开始就是支持的态度。其次绝大部分开发人员觉得在这个过程中能自己能学习到东西并没有抵触遇到很好的意见时大家都还是很高兴的。最后慢慢的形成了一种氛围整个团队都会自觉的维护它。附一张我们审核的对话图这位童鞋尝试对系统内部散落各地发业务邮件的代码做一个整理用一套模式来处理调整了3版才定调然后修改了很多细节才通过了合并前后大概用一个多星期时间表面上看来Code Review会延缓项目的进度但是在我们2年多的执行过程中大多数时候没感觉到有延缓。原因是虽然代码合并的周期变长了但是由于代码质量提高了导致Bug变少了由于Bug引起的返工问题也变少了因此整体的进度其实并没有延缓。我个人认为对一个成熟的团队其实做Code Review反而会加快整体的项目进度但是手头上没有统计数据支撑我的观点。对于软件开发的度量欢迎有心得的同学告知我我们每个分支有权限合并的人都不止一个这样可以保证有人请假不在的时候代码仍然可以被其他同事审核通过之后合并。半年前我们团队加入了很多新成员刚加入的新同事对规范、项目、产品的熟悉程度都不高导致了有一段时间我们遇到了PR审核周期变长的问题。加上之前遇到的一些问题我们总结了一个说明目的是减轻Code Review对开发人员工作的负担加快PR审核通过的过程。说明如下Pull Request 的说明任务完成才能提交PR。PR应该在一个工作日内被合并或者被拒绝。PR在有严重问题包括但不限于架构问题、安全问题、设计问题太多问题或者任务无效的情况下会被拒绝。严禁一个PR里面有多个任务除非它们是紧密关联的。PR提交之后只允许针对Review发现问题再次提交代码除非有充足的理由严禁在同一个PR中再次提交其它任务的代码。提交PR时候有一个描述框内容会自动根据Commit的message合并而成。切记如果一次提交的内容包含很多Commit请不要使用自动生成的描述。请用简短但是足够说明问题的语言理想是控制在3句话之内来描述你改动了什么解决了什么问题需要代码审查的人留意那些影响比较大的改动。特别需要留意如果对基础、公共的组件进行了改动一定要另起一行特别说明。审核人员邀请原则1. 在创建PR时Reviewers审核人一栏里主要填写“必需审核人”。只有这些人审核都通过才允许合并。2. 除了“必需审核人”外还有一些其它审核人我们可以在Description里做为“邀请审核嘉宾”进来。3. 主干分支间的合并如Develop Master或Master Develop等则需要把整个团队开发QA都列为“必需审核人”。必须审核人的列表由团队决定可能包括以下人选团队Leader前端架构师如果有前端代码改动 可以授权后端架构师如果有后端代码改动 可以授权产品架构师对此PR解决的问题比较熟悉的之前一直负责这部分业务的同事此PR解决的问题对他影响比较大比如认领的任务依赖此PR的同事其它审核人包括但不限于需要知悉此处代码改动的人但又不必非要其审核通过的同事可以从这个PR中学习的同事