Code Review落地方案


title: Code Review落地方案
date: 2020-05-31 13:32:31
update:
tags:

  • code review
  • 白盒
  • 推荐阅读
    categories: 研发效率

本文首发于我的个人博客

之前的博文也写过两篇关于Code Review的文章,出发点的各不相同:

似乎Code Review相关的话题特别容易水文章😅。其实网上有很多介绍Code Review的流程介绍,中英资料都不缺少,但是并没有一个更加系统的文章来阐述一个完整可行的Code Review方案,具体到流程的每一个环节。

相比于再水一篇烂大街的《如何做Code Review》,梳理一篇更系统的Code Review落地方案,给有需要的同学提供参考似乎更有趣也更有意义。本着这样的想法,我从QA的视野,尝试从研发效率的角度整理一些经验以及想法,其中有不少灵感是来源于当前所在的研发团队正在落地的Code Review方案,Code Review后面简称CR。


Code Review 背景

大家都知道Bug越早被发现,修复成本越低,各种自动化检查就是尽可能地让测试左移,在研发流程早期发现Bug。按照这个思路从研发流程的角度看,先有RD自测,然后是RD的提交代码前后的单元测试,接下来是代码合入主干分支触发的自动化测试以及CR。先抛开RD的自测与单测不说,这两个点想要做好时常难度较大,需要很强的推动力。那从这个角度看,代码合入主干分支的时间点基本上是外界介入测试的最早阶段,而CR正是一种早期发现Bug的手段,自然而然可以融合进来。

另一方面,CR落地情况体现了一个研发团队的工程氛围,新人很容易通过CR获得技术提升;同时,CR提升了大家对同一个模块的熟悉度,避免一个模块只有单一同学了解,防止人员流动导致代码开发维护上的真空期,好比分布式系统是不希望出现单点,master节点要随时可被替代一样,否则系统出现故障的概率就变高。

更进一步,线上事故经常有,我们的QA联合RD对一系列线上事故做分析之后,发现很多造成事故的Bug都具备在CR阶段被发现的可能性(当然不是100%能被发现)。

综上三点,已经阐述了Code Review的必要性。


QA如何做 Code Review

Code Review

QA为何难以参与CR

很多QA可能觉得CR是RD的工作,跟QA没有关系,这是片面的。CR确实是RD主导,但也不失为QA贯彻测试左移原则一个好的切入点,还能顺便看看产品代码从细节上熟悉被测对象,何乐而不为?

个人感觉QA难以在CR中体现出参与感的一个最关键的点,可能是很多QA本身缺少一定的代码经验或白盒测试思维导致CR参与度低,换句话说,就是缺少做CR经验和方法论,这些都是可以通过实践去积累的。老QA员工就不看重CR,上梁不正下梁歪,新来的QA同学很可能也不会看重CR。另一方面,确实是日常需求测试比较重,尤其是客户端(移动端和Web端)的同学,业务压力大,是客观现象,这就不在这里做讨论了。私以为,一个好QA,除了充当流程规范的把关者外,技术能力是不应该跟RD相差太远的。

我所在的QA团队做CR也一直有问题,如下:

  • 业务类需求(泛指常规具备交互因素的功能),大多数都是需求完成测试后才发起CR,CR的时间节点过于滞后,导致CR发现问题改代码后又缺失测试环节。正确做法应该在测试前或者测试中完成CR,或CR修改完后复测

  • 技术类需求(泛指代码优化或UI不好体现的功能),QA没有参与需求前期阶段,在不清楚上下文的情况下被RD拉进去对陌生功能模块进行CR,没有效果

  • 鉴于上面两点,QA很难发挥真正的合码卡口质量担保的作用,也没办法建立RD对QA在技术上的认可度,QA的CR流于形式(甚至RD的CR也是)

QA视角的CR目标

最终目标都是质量保障,CR是一种测试左移的手段,意图在测试前期发现Bug,降低Bug修复的成本。QA视角的CR其实是对RD视角的CR做补充,在关注点上会有区别。QA是以发现代码中的质量缺陷为首要目标,也就是找Bug,而不是代码是否优雅或代码有无更优实现方式

QA的CR准则

要明白QA在CR中扮演的角色,CR的关注点可以很多,如:代码设计、可维护性、可扩展性、安全、性能等,但并非要求QA面面俱到,作为质量把控的角色,QA应该更集中在功能实现和代码健壮性上。

1. 不纠结编码风格

  • 理想情况下,编码风格的把关应该交由专门工具(各种lint)或RD自己保证,QA的主要精力应该在代码逻辑上而非命名风格、函数长度、函数返回值等这类规范细节上。

2. 明确 Change List

  • 改动范围是QA判断CR策略的核心信息,QA对代码本身以及模块设计的熟悉度肯定不及RD,在CR时需要获取更多辅助信息协助CR。
  • 知悉改动范围可以在业务知识上先行识别代码风险,然后根据风险点从代码层面上逐个排查——可以理解为QA根据改动先设计 CR用例,再拿具体代码对着 CR用例 来找Bug,如针对一段循环逻辑,QA基于业务知识知道循环的结束条件,再结合具体实现寻找是否存在导致死循环的潜在条件场景。
  • Double check附带的说明文档,以防理解错误或遗漏信息。

3. 思考Bug在哪里

  • 请跟我默念:什么场景下这块代码会出Bug。QA CR关键是要做思维切换 —— 尽量刻意遍历、覆盖可能出Bug的场景。而不是顺着代码逻辑看下去有没有疑点,可能就被RD的思维同化了。比如看到一个if判断,就要思考这个判断变量的边界值、触发场景等。
  • 得先理解RD的实现思路,可能还是要顺着RD的思路先过一遍,理清功能细节与调用关系,再转换思维重看一遍。

4. 疑问找RD解答

  • 尤其是对于技术需求,大多数情况下不能保证QA的前期参与,QA不清楚需求背景与代码设计,存在严重信息不对称,无法google解决的疑问不要花太多时间琢磨,直接问RD的效率更高。
  • 不要觉得问实现相关的问题很羞耻,RD之间review代码都要互相了解实现细节,QA就更不用说了,比如特殊实现手法、语言高级特性、改动点涉及的调用关系等,都能找RD解答。
  • 如果大范围看不懂且该CR很重要,果断找RD当面了解,或远程语音。

5. 严格控制时间花销

  • 保证CR的速度,把控时间花销,很重要!!!特别是一个QA对接好多RD的需求这种现状,如果集成代码那天来5个Merge Request,一次CR半小时,那当天就没了快一半的时间(算上任务切换的耗时)。QA做CR以走读为主,关键代码细看即可,不必每行都看懂。
  • 不同的需求对CR要求不一样,平台强相关的进阶技术、大范围多文件改动、基于复杂功能的调整、UI需求等QA可能较难review,建议粗粒度CR;对于CR更友好的需求,如新增一个依赖少的独立小功能,QA可以获取到完整的上下文,应该多花一些时间CR。
  • CR来的时间不合适,考虑换一个具备相关能力的QA同学review。

6. 可以给RD提建议

  • 觉得存在遗漏的场景或未知风险的代码,应该指出来,可能是不知悉代码上下文带来的误判,也有可能是真的有问题!

do CR right

QA做CR的一些经验

RD做CR更多的是从风格、代码设计、注释、功能、性能、安全、可维护性、等各种维度下手,QA做CR更多是从功能、异常处理、性能、可测性方面下手,关注点会比RD少但是更集中。一般可以根据代码Diff,顺着数据流转链路往前后延伸走读,下面列举一些经验性的关注点。

可以让RD当面解释数据流转过程,有奇效。


🚨 QA重要考点

  • 正确初始化
  • 弱网、断网、失败
  • 缓存清理、缓存失效
  • 持久化数据被删
  • 变量判空
  • 越界
  • 循环结束条件
  • 数据格式与类型非预期(常见服务端线上事故)
  • 锁获取:饿死、死锁
  • 阻塞与非阻塞、同步还是异步
  • 内存/资源泄露,尤其是异常逻辑
  • 隐式类型转换
  • 上下游接口明确
  • 实现方案有缺陷
  • 安全问题(如关键信息明文持久化)
  • ab实验相关代码要了解开实验的详情

一个简单却又经常被忽略的辅助信息:历史commit message


代码可测性

如果需要QA做具体测试的需求,还要关注一下代码程序的可测性

  • 可验证性
    • 添加必要的日志
    • 因输入导致变更的点就是输出,输出全部可查看(正例:中间数据的落盘)
    • 提示信息可读,意义明确且唯一(反例:返回码意义不明确且多个不同错误用同一个返回码)
  • 可操作性
    • 简化测试准备和测试清理工作(正例:几个标志组合的判断条件,可一键完成标志设置,不需要人工操作场景完成设置)
    • 测试过程有易用的配套工具
  • 可控制性
    • 所有影响输出的因素都可控(反例:难以构造的死锁现场;难以构造的内部异常)
    • 可直接控制中间状态的数据
  • 可分解性
    • 大系统中可以针对小模块独立测试
  • 可理解性
    • 文档明确且随时update
    • 提供修改背景和影响范围

CR例子

这里给一些简单例子,可能不严谨,体会到意思即可。

一、变量没有初始化即使用

// 注意java基本类型默认有初始值,但是赋初值还是个好习惯
public class A{
    // ...
}

A a;
// ...
uploadData(a);

二、代码不符本意

mainDic已经初始化,if 判空逻辑永远为True,应该是 mainDic.count

NSMutableDictionary *mainDic = [NSMutableDictionary dictionary];

if (mainDic){
    // ...
}

三、异常情况遗漏处理 或 处理不完善

如断网弱网重试、异常抛出未catch、变量不判空。

// 文件资源在抛异常时没有close,资源泄露
try{
  OutputStream out = new FileOutputStream("");
  // ...
  out.close();
}catch(Exception e){
  e.printStackTrace();
}

四、循环结束判断条件错误

喜闻乐见你懂的,不用举例了。


五、手误写错变量名、函数名
int rulesForA(){
// ...
}
int rulesForB(){
// ...
}
A a = new A();
B b = new B()
rulesForA(b);   //  手误写错变量名or函数名
rulesForB(a);

六、全局变量与局部变量混淆使用
class Test{
  static int a = 0;
  public void test(){
    int a = 9;
    // ...
    a += 1;   // 可能本意是对全局变量a自增,这里错误把局部变量a自增
  }
}

更多关于做CR的方法论,谷歌也出了一个CR工程师实践,文末参考资料附上链接。


Code Review 配套工具

流程平台

想要好好做CR,必须要依托工程平台的支持,你不可能把别人提交的分支代码拉到本地来diff着看吧,所以针对CR环节也对工程平台提出了一些基本要求:

  1. 不同的聚合方式来查看diff:
    • 改动涉及的不同仓库(改动涉及多仓,分开看)

    • 改动的文件类型(如统一查看.java后缀的文件改动)

    • 被修改文件的组成的目录树

  2. diff代码颜色高亮展示,支持查看diff代码以外的其他代码
  3. 支持任意代码添加CR评论 以及 展示评论的解决情况
  4. 展示source分支的历史commit message
  5. 展示source分支以及要合并到的target分支
  6. 绑定文档模板,让RD按模板填写相关内容
  7. 查看所有需要待CR的Merge Request

精准pick人

相信有参与CR流程的同学都会遇到同一个问题,就是在需求扎堆上车合码的日期,当天会收到大量CR邀请,不说手头工作进度被拖累,收到的CR邀请可能还跟自己负责的模块不相关,尤其是QA同学对接多个RD,可能都会被不同的RD邀请CR。所以这个问题必须要解决:如何更精确地pick CR候选人

相比于随机选择CR人选,有一个十分简单的解决办法:通过后台配置,指定某个代码目录匹配相关CR人选。一个代码目录应该有一到两个负责人,再加上若干候选人,CR的时候自动拉负责人,候选人则随机拉若干个,这样基本上就解决了随机选择CR人选导致CR不相关效果差的问题。

但是随着模块越来越多,团队越来越大,人员流动与代码变更会变得十分频繁,上面提到的配置名单是否还方便维护呢?答案肯定是No,每来一个新人,就要将这个新人添加到配置里,太麻烦了!还需要一种更精准的自动推荐CR人选的方式,这里我有一个想法:

流程总览

  1. RD完成代码开发,提交代码发起Merge Request
  2. 根据RD改动的文件,结合 该文件与其他文件的 关联关系,获取被修改文件相关的其他文件,层级上下共x层(可配置)
  3. 遍历这些关联文件,逐一找出这些文件模块的RD(如果是子repo,获取repo owner;如果是子文件,根据git blame等git命令获取文件的最新(or最频繁)改动RD,否则兜底到根据配置获取文件负责人)
  4. pick这些RD进来CR代码,或者推荐给发起CR的RD让ta自己选择拉谁

一个具体例子

  1. RD小陈修改了代码文件A.java,提交后发起Merge Request
  2. 分析A.java的改动,发现本次改动中添加/删除/修改 方法function,根据A.java的代码进行分析,方法function属于类Example,类Example在B.java中被定义声明,被A.java import了进来;A.java在文件C.java、D.java中也有被使用,而C.java、D.java里定义的类被E.java和F.java调用。所以最后获取到关联的上下游文件集合有 [ A.java, B.java, C.java, D.java, E.java, F.java ],下面附一个简单的图来表达这个上下关联关系
  3. 获取 [ A.java, B.java, C.java, D.java, E.java, F.java, G.java ] 这些代码文件的CR候选人(这里可以根据git代码行修改人获取对应RD,也可以配置RD)
  4. 这些候选RD推荐给CR发起人,让他选择拉哪些RD进来CR

代码文件的关联关系举例

核心问题

如何抽取出代码文件之间的关联关系?使用什么样的静态代码分析技术?

以前有接触过类似的项目,通过正则表达式匹配不同的java语法,匹配范围包括:类对象的创建声明、包的引用关系,这样就可以知道一个文件会对外提供什么类,以及它使用了来自什么包的哪个类,进一步地,就可以匹配出两个java文件之间是否有关联关系——你调用我还是我调用你,也就是上下游关联关系。比如A.java有类A,B.java引入了类A并进行使用,那么B.java与A.java就存在关联关系。

基于正则的代码分析方案会有非常多坑,一方面是语法很难通过正则来穷极表达,难免出现语法遗漏覆盖导致关联关系的丢失;另外还需要处理大量代码格式兼容问题,代码格式稍有变动(比如敲多俩空格,代码中间换行展示)就可能导致正则匹配miss,又会出现关联关系丢失,所以不建议采用正则来做分析。

业界更常见的做法,是基于parser获取的AST(Abstract Syntax Tree)分析文件的关联关系。

对于移动端代码,本身又有很多系统平台提供的回调函数(比如Android onCreate方法等),这些在关联关系上还需要做特别处理。

其他可能的问题

  1. 关联关系的分析性能,能否做到实时分析?使用线下分析的关联结果准确率多高?
  2. 大范围的改动,比如代码沉库、代码迁移,是否也适用?
  3. 打通代码库提交历史,具备静态代码分析能力以获取代码文件之间的关联关系,根据修改历史

Code Review 落地

Code Review流程大家应该基本清楚,那么CR应该如何落地,下面展开讲述一下。

理念宣讲

CR落地,第一件事就要宣讲CR理念,让大家知道这么一件事情的存在。最关键是要讲清楚为什么要做CR、CR如何嵌入当前流程、大家需要比平时额外做一些什么以及如何做CR,最后表达CR正式落地后的预期收益。

流程试点

研发流程改动是一件大事,起码CR的加入,涉及到RD与QA的工作流程变动,上上下下就是几十甚至上百人,再不能100%确定可以获取足够的收益前,流程试点是必须的。

不得不提,CR本身虽然只是一个流程环节,但是CR环节效果的好坏跟前置流程直接相关,比如需求评审的粗细、技术文档的质量、代码注释的多寡等。

在CR全面推广普及之前,CR可以有多种试点落地形式,比如:

  1. 做一份人员配对名单,几个相关RD为一组,互相review对方的代码
  2. 筛选部分需求放到CR池子,RD选择想要review的需求自行参与CR

两种形式各有优缺点,但是第二种形式给了更多自由,可能参与度更高,但是也要避免需求的扎堆Review,尤其是技术性很强很高大上的需求,毕竟做工程师很多都会对高深技术有向往,每个需求要设置一个CR上限人数,超过人数后就从CR池子中剔除。

奖惩机制

正向鼓励可以加速CR氛围的形成,最终要以实物作为奖励(京东卡?),所以需要一些预算💰,根据大家的产出来授予奖品。至于惩罚理所当然就是事故共担,线上问题责任按比例分配。

奖励考虑通过积分来兑换,所以可以做一个CR积分排行榜,具体分数可由如下几项指标转换过来:

  • 主动评估
    • reviewer评估本次CR给自身带来的收获打分(对模块的熟悉度、代码技能的提升等)
    • CR效果打分(reviewer在CR结束后打分,评估本次review效果)
  • 被动评估
    • CR前置发现的问题数
    • 千行代码Bug数(可以按不同维度细分:团队、个人、需求等维度)
    • 单个需求的Bug数
    • 有效的CR评论(标记为已解决的评论)

写在最后

从各种资料看,国外大厂似乎比较流行Code Review,也许这使得国内对Code Review的态度时不时会有种难以言状的推崇或者仰望,错误地认为Code Review可以解决很多问题。Code Review仅仅作为一种质量保障的补充手段,它并不是银弹,不要指望Code Review可以发现多少关键问题,即使投入无限的人力也只能获取有限的收益,它的效果不如测试那么稳定。

这不代表就不需要Code Review,长期来看一个健康的技术团队是需要实践Code Review的。尤其是大体量产品,体量越大Bug产生的影响也就越大,而Code Review的性价比越高,为什么这么说呢?

试想一下产品的日活从100W变成1000W,同样症状的线上事故,影响的用户量整整相差一个量级,也就代表着100W日活时一个P2级线上事故,在1000W日活时它会上升为P1级甚至P0级。

产品代码越复杂,Bug就隐藏得越深,问题场景就越构造,线下人工或者自动化测试,碍于工具支持以及建设程度,总有难以覆盖到的点,而Code Review正是一种精细耕作的手段,以弥补测试的不足,特别是很隐晦的Bug,比如资源竞争导致死锁饿死、异常处理分支出错、大流量下的性能问题、技术债的有无(技术债也是一种问题,未来需要成本解决)等……这些就是Code Review独特的作用。

随着产品体量增大,Code Review能发挥的价值会有所增长。

Code Review做得不好,就说Code Review无用,这样的说法就好比单元测试写得烂,就说单元测试不用做一样


参考资料

从 CODE REVIEW 谈如何做技术

Code Review方案

Google Engineering Practices Documentation

Code Review Guidelines for Humans

posted @ 2020-06-13 14:11  拉丁看雪  阅读(480)  评论(0编辑  收藏  举报