ARTICLE DETAIL

资讯详情

深耕网站视觉设计与运营推广的一线实战洞察。

从“形式审查”到“开放协作”:代码审查流程改造实践

从“形式审查”到“开放协作”:代码审查流程改造实践 1. 为什么我最终把代码审查做成了开放式的先说一个我自己踩出来的结论代码审查这件事开放程度决定了它到底是质量保障手段还是团队内耗源头。早年我在一家小团队带项目代码审查基本靠领导抽检后端互看审查结果只出现在周报的某个角落里。看起来流程也有实际上大家心里都清楚那只是走个形式。后来在一个开源风格浓厚的团队待了一阵子我见识到了完全不同的方式每一个合入请求都开放给所有人看任何人有疑问都可以直接在评论区提出讨论公开、意见透明、修改过程全程留痕。刚开始我非常不适应总觉得这不就是把我们组的内部问题暴露给所有人了吗但跑了一段时间后我服了。开放式代码审查open-code-review核心就一句话把代码评审从个别负责人把关变成团队协作共建。它不要求所有项目都开源而是指评审过程本身开放任何人可看、可评、可追问、可记录。围绕这个思路我把自己的项目组从形式审查逐步改造成开放审查这篇文章就是整个改造过程的全记录。我会尽量少讲虚的把我在设计流程、选工具、定规则、踩坑修复过程中获得的经验都写出来。适合正在做团队工程效率改进、想要落地代码评审机制或者对如何让代码审查不流于形式这个问题有疑惑的人参考。2. 开放式代码评审的基础设施与流程设计2.1 评审入口的统一合入请求是唯一的评审单元很多团队的评审混乱根源在于评审入口不统一。有人用邮件发 diff 给人看有人在群里一下帮我看看这个有人等代码进去了才补个记录。这样的结果是评审无历史、意见无归集、修改无跟踪。我在改造时做的第一件事就是规定所有变更必须走合入请求Merge Request / Pull Request这是整个开放式评审的底座。这不是什么新鲜做法但真正严格执行的团队并不多。理由很简单合入请求天然带有完整的 diff 视图评审者能看清每一行改动。讨论、评论、提交记录、流水线状态会全部挂在同一个客体上信息不散。合入操作可以被权限管控评审未通过时不允许合并这就在机制上挡住了先合再说。这里要强调一点合入请求要小。如果一个合入请求改了十几个文件、涉及三四个功能点评审者看一遍消耗的精力极大最后结果通常是草草点个 approved。我在团队里明确要求单个合入请求原则上不超过 400 行改动偏重构类的不超过 800 行特殊情况要主动拆分。这个数字是参考了很多开源项目实践后定的不算严苛但能逼着大家把大变更拆小。拆小的直接收益是评审周转变快了。以前一个大型功能合入请求挂三四天是常态现在中等改动基本上当天就能走完评审。这对团队的实际效率提升非常明显因为代码在等待评审期间其实处于冻结状态挂得越久牵扯的上下文切换成本越高。2.2 分支策略怎么配合评审机制分支策略会影响评审的顺畅程度。我建议按团队成熟度选择不用一上来就上最复杂的 GitFlow。对于多数中小团队我推荐Trunk-based Development 的轻量变体每个人从主干拉出功能分支开发完成后发起合入请求评审通过后直接合回主干。主干永远是稳定的因为每一次进入主干的变更都经过了公开评审和自动化检查。这里有一个细节很多人会忽略**分支的存活时间。**如果一条功能分支活了两三周合入时的冲突会让人崩溃评审的 diff 也会被大量无意义的合并噪音干扰。我定的规则是功能分支存活周期不超过三到五个工作日超出必须主动与主干同步。用来保证这条的做法是鼓励频繁发起小合入而不是万事俱备才开评审。主干保护策略上我开启了两个硬性规则必须至少 1 个非作者的评审者点 Approve 后才允许合并。所有自动化流水线必须通过编译、lint、单元测试、覆盖率检查。这两条规则不是说把门槛锁死而是给开放评审兜底。既然任何人都可以参与评审那么至少得保证基础质量门禁是机器在守人工评审负责的是机器管不了的部分。3. 评审角色、粒度控制与检查清单的落地实践3.1 谁负责评审不用等官方评审人开放式评审最容易走到一个极端开放到最后变成没人评审。每个人都以为别人会看结果谁也没看。针对这种情况我给团队定了三种角色每种角色职责不同角色职责数量建议作者发起合入请求回答评论推动修改1 人指定评审者对本次变更加载责任心必须在约定时间内给出结论1~2 人按功能复杂度自由评审者利用碎片时间浏览、提问、补充意见不设上限指定评审者按照什么逻辑指定不能永远找代码写得最好的那个人否则他必然成为瓶颈。我的建议是涉及核心模块的改动必须指定该模块的负责人或熟悉该模块历史的人。涉及跨模块影响指定端到端链路的下一环负责人比如你改了数据表结构就指定负责写查询层的人。新人的代码指定一位有经验的成员做 Mentor 式评审重点不是挑错而是解释为什么。自由评审者的价值在于视角互补。一个功能由后端实现前端同事从接口调用角度看可能会发现响应字段命名不清晰测试同事从验证角度可能会直接问这个分支分支条件怎么测不到。这类意见往往比指定的同组评审更有价值因为它是来自真实使用场景的反馈。3.2 评审粒度逻辑正确排在第一位风格问题交给机器很多团队评审像文字校对一边看逻辑一边抓空格、命名、行长度。这其实是评审效率低下的重要原因。人的注意力是稀缺资源用在低价值问题上是巨大的浪费。我们执行的原则是人工评审只解决机器解决不了的问题机器能解决的机器解决。人工评审重点关注这些业务逻辑是否符合需求预期边界条件是否覆盖。是否有潜在的并发问题、事务边界错误、资源泄漏。异常处理是否合理错误信息对调用方是否有意义。引入的依赖与架构方向是否一致是否存在过度设计。测试是否有效覆盖了核心路径测试本身是不是在自我欺骗。为了不让评审者漏项我准备了一份尽量精简的评审检查清单放在仓库的CONTRIBUTING.md里。新成员参与评审时可以照着过一遍习惯了之后自然内化# 评审检查清单 ## 逻辑与正确性 - [ ] 核心路径是否与需求文档一致 - [ ] 是否考虑了空值、越界、重复调用等边界 - [ ] 并发场景是否存在竞态条件 ## 异常与恢复 - [ ] 失败路径是否有清晰处理 - [ ] 错误信息是否包含足够上下文 ## 可维护性 - [ ] 命名是否准确表达了意图 - [ ] 是否复用了已有组件而非重复造轮子 - [ ] 新增代码是否有对应测试 ## 性能与安全 - [ ] 是否避免了N1查询/循环内IO - [ ] 新增输入是否做了校验这里要特别说一个我见过的高频问题评审者容易陷入风格辩论。A 觉得这种写法好B 觉得另一种写法好双方都是对的但讨论三天毫无结果。我的处理办法很干脆风格问题一律交给格式化工具和 lint 规则裁决不纳入人工评审讨论。团队里形成不成文规定对风格有意见请改配置文件发起提案不要在合入请求评论区里争。3.3 Review 轮次怎么控制避免无限循环评审最怕一种情况作者改一版评审者提新意见作者再改评审者又提新意见…… 来来回回十轮两个人都精疲力尽。我的经验是给评审轮次设一个收敛预期第一轮评审评审者全面提意见包括逻辑问题、遗漏场景、测试不足。作者统一修改后进入第二轮。第二轮只验证一轮意见是否全部解决如果出现全新的、未被提及的修改点默认不是阻塞项可以记录为一个后续任务但不应继续挂起本次合入。这样做的主要理由是**没有完美的代码只有不断逼近完整的系统。**把一些小问题做成后续 issue 合入后再修整体推进效率远高于在一个 MR 上死磕。4. 自动化规则与人工评审的分工边界4.1 哪些检查必须交给机器如果人工评审沦为抓 lint 错误的工具那说明自动化建设严重不足。我在搭建评审体系时把下面这些检查全数接入了管道的必查门槛自动化检查类型具体内容失败处理格式检查Go fmt / Prettier / Black 等统一格式阻止合入静态分析ESLint / golangci-lint / pylint 常见规则集阻止合入单元测试覆盖核心模块的测试用例阻止合入覆盖率门禁新增代码覆盖率不低于 70%阻止合入构建验证编译通过产物可生成阻止合入为什么覆盖率门槛必须存在因为它是防止代码只是写了测试这一情况的兜底。开放评审里如果评审者还得分心去看这个测试到底执行了多少行代码工作量就太大了。覆盖率数值是一个粗糙但极其有效的过滤器它能把明显没写测试的情况筛掉具体测试写得好不好再交给人工。我在实际配置里用过 GitHub Actions 做这套检查也用过极狐 GitLab CI 和轻量的 Drone本质都是一样的。给一个参考片段这里用 GitLab CI 的.gitlab-ci.ymlstages: - verify - build lint: stage: verify script: - npm ci - npm run lint only: - merge_requests test: stage: verify script: - npm ci - npm run test:coverage coverage: /All files[^|]*\|[^|]*\s([\d\.])/ only: - merge_requests build: stage: build script: - npm run build only: - merge_requests注意流水线最好配上only: merge_requests或是相应的分支条件避免每次 push 到远程分支都触发整套流水线。因为没有合入目标时跑这些检查的参考价值有限还浪费资源。4.2 自动化与人工如何在流程中衔接衔接的核心要点是**自动检查先跑完人工评审再介入。**Pipeline 是红的时候评审者不需要浪费时间看代码直接挂个 pending 等开发者修。这个规则在团队里要写入工作协议否则就会出现评审者花了半小时看完代码结果发现编译都没过的情况。另一个衔接点**自动化能帮评审者定位问题范围。**我实际上非常推荐在合入请求描述里加上变更影响范围的自动提示比如用脚本解析改动文件列表自动标注本次变更涉及模块鉴权、订单、权限。这样一来指定评审者不用自己费劲推断改动影响范围可以直接根据提示判断自己是否是这个模块的合适评审人。小团队可能觉得这步没必要但团队成员超过 10 人、模块划分清晰之后这个自动提示能显著降低找错评审人导致评审质量不高的概率。4.3 自动化配置的几个容易踩的坑我遇到的第一个坑**覆盖率门禁用全局覆盖率结果新代码覆盖率极低也被放过了。**比如项目原本全局覆盖率 85%一条修改只给新加的函数写了冒烟测试覆盖率 30%但全局算下来还是 84.3%门禁过了。正确做法是只检查本次变更新增代码的覆盖率GitLab 里有coverage报告的 diff 能力GitHub 上可以用 peter-evans 这类工具或者 Codecov 的patch配置实现。第二个坑**lint 规则频繁变动导致存量代码大面积报错。**刚引入静态检查时建议先设成 warning 级别只把 error 作为合入门禁。等团队适应了再把 warning 逐批提升为 error。我见过上来就把几百条存量问题全部标红阻塞合入的团队结果大家只能全员停下手头工作修格式整个过程非常痛苦。5. 开放式评审推进中的实际问题与对策以下建议是我在从封闭评审转向开放评审的过程中实际遇到的重重麻烦与处理方法可以说直到今天它们仍然是一般团队推进评审机制的最大绊脚石。5.1 问题一代码评审排队严重MR 挂一周没人看这是开放式评审施行初期最普遍的问题。指定评审者自己活儿都忙不完哪有时间看你的代码结果上线的节奏被评审卡得死死的。我试着把评审等待当成一个显性的流程问题来处理而不是靠呼吁自觉。方法如下把评审时间排进项目计划。每个迭代预留 15% 的产能专门用于评审他人代码而不是让评审插空进行。规定合入请求创建后 4 个工作小时内必须有人响应。响应可以是正式的评审意见也可以是我 2 小时内看的承诺。没有得到响应的请求会由项目机器人自动提醒。超过 8 小时无人评审自动上报到周会讨论。这一条很少真正走到但只要存在就会促使组长们优先分配评审资源。这个制度实施一个月后合入请求的平均首评响应时间从 2.3 天降到 5 小时以内效果非常显著。5.2 问题二评论区变成战场互怼取代了交流开放意味着更多的声音也就意味着更容易出现对抗性沟通。我遇到过一次冲突起因是 A 写了段比较取巧的兼容逻辑B 在评论区说这部分明显有隐患建议重写。A 回复这不是隐患你对这个模块的历史背景不了解两个人越说越僵。最后虽然是技术问题但气氛变得非常僵硬。从那以后我制定了三条规定写进团队约定评论要给出为什么而不是只下结论。建议永远搭配理由或替代方案。允许反驳但禁止评价人。只能针对代码和方案说话不说你写的代码质量差。评审意见在评论区无法达成共识时约定 15 分钟会议室拉齐。打开视频看着对方的脸沟通语气会柔和很多。开放式评审最怕的不是有反对意见而是没有反对意见但有隐性不满。如果能保持透明、技术化的讨论氛围再激烈的争议都在可控范围内。5.3 问题三新人不懂评审怕被公开处刑注意开放评审对资深成员是信任对新人却是压力。一个入职三周的新人发起合入请求满屏评论都是改进建议哪怕意见都合理心里也会发慌。我的解决方案是双轨制新人的首个合入请求指定一位 Mentor 先行离线过一遍代码把明显的问题提前沟通掉再走公开评审流程。公开评审里剩下的问题是少数、且都是值得讨论的。在公开评审中对新人代码的评论要附加这是学到的东西不是对你的否定一类的语境。虽然这看起来是在管教说话方式但确实能显著降低新人适应成本。另外资深成员在评审新人的代码时我会刻意引导他们把必须修改和可选建议区分开。可选建议一律标注为nit:前缀不阻塞合入。这样新人能识别出哪些是硬性问题哪些是自己可以后续优化的方向。5.4 问题四评审意见有没有人跟踪没有闭环很多团队花大力气建立了评审流程却忽略了意见的闭环管理。有同事在评论区提了意见作者说哦好的谢谢然后合入了意见实际没有改——这比没有评审更糟糕它会迅速消耗评审者的参与意愿。针对这个问题我设了一个非常简单的规则**合入前发起人必须逐条回复评论未修改的必须给出理由。**这个规则在 GitLab 和 GitHub 的空状态支持上没有原生强制力我通过一个小机器人来实现合入时检查所有 comment 是否处于 resolved 状态存在未处理评论则拦截合入。这个自动约束执行后没有出现谢谢了事的情况每一条意见都有了明确的处理结果。跟踪的下一步是评审意见的周期性复盘。每个月我会让每个开发整理这个月评审中提出的最有价值的一条意见聚合后在全组分享。这既是知识沉淀也让评审这项工作有了看得见的收获避免长期执行后变成机械流程。6. 让开放式评审从流程文件变成团队习惯的几点体会6.1 不要把评审当成上线前的关卡而是当成写完代码之后的一个步骤我观察到过一种典型心态开发者把发起评审理解成提交给安检甚至存在一种希望一遍过的执念。如果评审不通过就觉得是审核失败情绪上产生抵触。我比较建议灌输每一条评审意见都是一次免费的同行设计咨询这种思路。评审不通过说明设计里存在一些未考虑到的维度这并不可怕。真正可怕的是这些维度没有被发现直到线上出问题才暴露。所以我在团队里的表述是**完成任务的时刻不是 push 代码到分支而是合入请求被 merge 的那一刻。**在这个时刻前所有反馈都是正常的协作过程。6.2 从开放评审延伸到开放设计代码评审做到一定程度你会发现很多问题的根源不在代码而在方案设计阶段。两个人吵得不可开交的合并问题如果设计阶段就讨论清楚了代码里根本不会出现。因此在代码评审稳定运行两个月后我们把它向前延伸了一步**重大功能在写代码之前先发一个设计文件Design Doc到团队频道同样以开放的评论方式讨论。**这个设计文件的评审不涉及具体代码行只关心方案的可行性、取舍和边界。这条延伸出去之后功能开发的一次通过率大幅提升。更关键的是代码评审的主题越来越聚焦因为大方向的问题在设计评审阶段就已经消化掉了。6.3 度量指标用什么衡量开放式评审做得好不好最后给我的三个核心度量指标比较适用于一般规模团队的数据参考指标统计口径健康目标首评响应时间合入请求创建到第一条评审意见 4 小时评审吞吐时间创建到合并通过 24 小时评审参与率非作者评论人数占团队人数比不低于 40%我见过不少团队过于关注每位开发者平均每周被提多少条评论稍微一想就会发现这个指标容易诱导偏差——评论越多不等于代码质量越高可能是代码风格不规范也可能是评审者在刷存在感。应该关注的始终是协作效率和心智负担而不是评论数量。最后再分享一个操作技巧把团队的评审约定写进仓库 README 最显眼的位置并让它成为新人入职资料的第一份文档。开放式评审与其他流程的最大区别在于它是靠文化而非靠行政指令来维持的。只要新人从第一天就对开放、透明、对事不对人有基本共识这套机制就能在人员更替之后依然稳定运转。
返回列表