白盒测试 [质量提升之道]-Code Review

扫地僧 · March 18, 2018 · Last by carter replied at August 22, 2018 · 4005 hits

躺在床上了,突然想到很久没写文章了,写什么呢?要不来个质量提升系列吧,就这样深夜爬起来了。。。

前言

互联网公司发版节奏快,对于兜底(背锅)的测试来说,压力真的很大,不但是考核的问题,还要面对各方指责。
在这浮躁的时代,评价一个优秀的测试人员,标准不是技术有多牛,开发的工具有多炫,职位有多高,收入令人羡慕。而是,如何务实、用心的为提升效率和质量,如何交付高质量的版本而努力工作。还是刚入行那句话,不忘初心,方得始终。

提升质量,先分析缺陷是如何从出生到线上的

1.开发人员没有进行有效的单元测试,带着一大堆问题提交;
2.代码没有进行Code Review,带着一大堆问题入库;
3.版本没有提测标准,带着一大堆问题到测试环境;
4.测试人员没有充足的时间,基于黑盒很难覆盖到每条路径;
5.生产部署出纰漏,线上没有进行快速有效的回归。
1~3都是基于白盒,对于质量就是阿喀琉斯之踵,这部分没做好,缺陷会犹如源头之水,接踵而至。
本文要说的是第2点Code Review。

学会阅读代码

写自己的代码容易,读别人的代码好难。我想说的是,掌握了方法,其实不难。
1.首先,要了解开发对于需求实现的设计方案,了解架构图、流程图、时序图;
2.其次,了解工程设计的层次结构,找到程序的入口;
3.然后,根据找到的入口,就可以从上往下,层层展开了;
以一个典型的后台服务组件为例,直接对外提供服务的是Controller层(程序入口),再往下是Service层,负责业务逻辑,再往下是DAO层,负责数据操作。当然,也会有一些基于AOP的横向切面逻辑,一般作用于Controller层,做一些防重、拦截等特殊性处理。还有一些是公共方法、工具类方法等。

有效的Code Review

在review代码前,要有自己的工作重点,并根据常见问题建立常规检查点。
以下2点需重点review
1.影响主流程跑通的类和方法;
2.多个地方调用到的,影响面大的公共方法;

Code Review常规检查点
以下所列只是常规检查点,实际操作不仅限于以下几点。
1.异常处理:是否需要捕获异常,捕获异常了有没有处理,捕获未处理的有没有往上抛,往上抛的有没有被上层处理;

 // 向上抛出异常
public class Demo {
public void testException(Object obj) {
if (null == obj) {
ServiceException serviceException = new ServiceException("666", "test");
throw serviceException; // 向上抛出捕获的异常
}
}
}

// 未捕获异常
public class Test {
public String testDemo(Object obj) {
new Demo().testException(obj); // 未捕获异常
return "test";
}

public static void main(String[] args) {
Test test = new Test();
String str = test.testDemo(null);
System.out.println(str);
}
}

2.补偿机制:是否需要补偿机制,补偿机制是否合理;
举个例子
完成一个方法有A、B、C三种方案,正常情况下走默认的A方案,如果遇到xx问题,走备选方案B,如果遇到xx问题,走备选方案C。同时,还要鉴定备选方案B、C是否合理,是否为最优方案。

// 下段代码的补偿机制使用了默认值,更好的方案是redis遇到异常,channel返回null,channel尝试去数据库取值,db遇到异常再考虑默认值
private static final String DEFAULT_CHANNEL = "default";

public String loadChannel(String key) {
String channel = null;

if (StringUtils.isNotEmpty(key)) {
try {
channel = redisUtil.get(key);
if (StringUtils.isEmpty(channel)) {
channel =mapper.loadKey(key);
if (StringUtils.isNotEmpty(channel)) {
redisUtil.set(key, channel);
}
}
} catch (Exception e) {
// 如果redis服务发生异常无法正常使用时,channel使用默认值
channel = DEFAULT_CHANNEL;
}
}
return channel;
}

3.NPE问题:某些赋值或条件,如果不进行适当的健壮性处理,会存在空指针的隐患;

// 如果a.getResponseCode()为null,报NPE
if (a.getResponseCode().equals(Constant.CLIENT_RESPONSE_BODY)) {
}

// 常量放前面则不会报NPE
if (Constant.CLIENT_RESPONSE_BODY.equals(a.getResponseCode())) {
}

4.大对象IO:某些地方如果频繁进行大对象的磁盘IO,可能对性能造成影响,程序员容易在打日志的时候疏忽这个问题;

log.info("linsence入参:{}",dto);  // 此处打印图片的Base64码,耗费磁盘资源,也影响IO性能

5.SQL问题:业务层面,需检测操作的数据,如查询的数据是否符合业务预期。性能方面,需检测该语句是否存在性能问题;
6.隐私安全:对于金融产品涉及的账密、证件、手机、银行卡等敏感信息是否进行了脱敏处理;
7.资源浪费:浪费内存资源、cpu资源等情况;
例如:对变量赋值,而该变量从未使用、字符串用+连接、对象无意义的new了一个实例等。

Demo demo = new Demo(); // 此处无需new对象
demo = CreateDemo.invoke();

8.参数校验:重点关注Controller层的方法入参,是否如接口文档设计进行了必填项、取值范围、长度、类型等校验;

@AccessControl
@NotRepeatable
@ApiOperation(value = "xx", notes = "xx")
@RequestMapping(value = "/test", method=RequestMethod.POST)
public ResponseDto<?> demo(@RequestBody RequestDto<TestDTO> dto) {
String xx= service.save(dto.getData()); // 没有在控制层对参数做校验
......
}

9.内存泄露:使用的资源未释放,可能导致内存泄露,如流文件未正确关闭等;

DataOutputStream outputStream = new DataOutputStream(connection.getOutputStream());  //如果是高频调用的方法,outputStream没有关闭流,存在内存泄漏隐患

10.错误码设计:糟糕的错误码设计和错误描述,不利于今后的问题定位。

ValidationError("100101", "ValidationError"), // 错误描述不明确

Code Review的价值

1.代码阶段发现和修复问题,效费比是最高的;
2.白盒发现的一些问题,往往是黑盒很难发现的;
3.对研发人员形成隐形的威慑,不能再随意的写代码了;
4.能间接提升版本的提测质量;
5.提升测试人员的核心竞争力。

Code Review的利器

Gitlab:只适用于git仓库。
Phabricator:支持git、svn等,真正做到了提交前的在线code review,后续会单独介绍。

后话

code review是提升质量的重要环节,一般以研发人员主,测试为辅共同完成,研发不做,测试有条件的也可以做。由测试提出的问题,需要研发人员配合修复,从个人经历而言,自下而上大都不会被重视,自上而下推动,可能落地效果更好。笔者认为,记录下发现的所有问题,问题应验了,自然会改变研发对测试的固化思维。

对质量而言,code review不是终点,只是开始。

如果觉得我的文章对您有用,请随意打赏。您的支持将鼓励我继续创作!
共收到 13 条回复 时间 点赞

楼主说得很在理,我们也在坚持做review,但发现收效甚微。大多都是规范类问题,很少有逻辑问题,很鸡肋的样子,怎么破?

许雯 回复

如果大多是规范类的问题,说明你们的研发团队个人能力很强啊,规范类问题是锦上添花的事,需强制推行的流程规范去约束

我们这边的研发提交代码之前要相互review,不然不能merge

这文章再配上代码示例就不错了,以前在雪球也是这样限制研发合并代码的。国内的code review目前全是人工,是非常考验专业能力的,所以普及度不太高。一般比较大的公司会有专门的团队负责这个。

自动化分析上目前也只有sonar和findbugs会做自动化的代码审计和静态分析,还不够智能。sonar提供了规则定制貌似还不错,findbugs也有定制化的方法可惜只能用于jvm语言。IDE等工具也自带了这些,但都是直接分析出隐患而没有给出语法树建模数据,导致没法定制。我记得以前有人曾经有人抽离了eclipse的语法分析工具,是个挺不错的思路。

挺希望行业可以出现一种根据代码流数据自动分析代码变更影响范围、发现分支逻辑问题、发现异常处理不充分、潜在的NPE等等问题。目前还没发现有什么比较好的开源工具,可以基于语法树做流程的自动分析和定制。如果有了语法树数据就可以对业务做更细致的建模,就可以深度的定制发现业务bug的规则了。

文章棒棒哒

我最近也在写一个公司的分享PPT 主题就是Codereview-初探
哈哈 好多楼主的东西可以直接放进我的PPT咯😁

FelixKang 回复

希望对你有帮助

已添加代码示例,code review和静态分析、扫描还是有区别的,后者不能发现关于需求实现是否和预期一致的业务逻辑的问题,而前者可以发现,这点工具目前是无法替代的。

#8楼 @quqing 也就是说 人工的review是任何自动化都无法替代的

—— 来自TesterHome官方 安卓客户端

除了phabricator 还有哪些?我觉得reviewboard也是不错的选择

—— 来自TesterHome官方 安卓客户端

扫地僧 #11 · March 22, 2018 作者
rockyrock 回复

我是这么认为的,人工智能足够成熟之前,工具不会知道业务需求该如何实现才是正确的。

扫地僧 [质量提升之道]-UT&Coverage 中提及了此贴 22 Mar 17:42
扫地僧 [质量提升之道]-Phabricator 的安装 中提及了此贴 09 Apr 15:14

我是比较推崇code review的,没有code review感觉上线的代码都存在风险。
只是推广起来太考验技术人员的能力。
我们公司一直在做code review,至少我的测试的工作都是基于code review进行的

扫地僧 回复

是的。我觉的测试人员在转测前的code review是很难在整个业务流程层面串联起来进行分析;反而开发人员交叉code review会更有效率。而测试的code review除了在测试之前进行外,业务层面的review更多的可以从发现bug之后来反推定位分析关联业务影响,以及空闲时间对整个被测业务的代码逻辑梳理来补充单纯 code review的不足;

扫地僧 [质量提升之道]-接口测试 中提及了此贴 06 Dec 00:30
需要 Sign In 后方可回复, 如果你还没有账号请点击这里 Sign Up