乐趣区

Code-Review落地方案

jiezi

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

  • code review
  • 白盒
  • 推荐阅读

categories: 研发效率

本文首发于我的 个人博客。

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

  • QA 测试是否要看代码——QA 读产品代码的必要性
  • Code Review 的思考——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

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 提建议

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

    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

    退出移动版