单元测试与问题发现
通过查看源代码,会发现一些潜在的问题。但为了验证这些问题确实存在,以及说明发生在何种场景以便测试重现,我们需要先对Think\Log这个类编写对应的单元测试,这也利于对后期进行重构后进行回归测试。
测试环境部署
如下所示,在ThinkPHP同级上建立了对应的测试目录test_ThinkPHP:
Application composer.json index.php Public README.md test_ThinkPHP ThinkPHP
以及最终的测试目录文件如下:
~/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP$ tree
├── build_phpunit_test_tpl.php
├── DbMock.class.php
├── Mock.class.php
├── test_env.php
├── Think_Log_Test.php
├── Think_LogV2_Test.php
└── ThinkPHP
├── Think.class.php
└── ThinkPHP.php
由于实际运行的环境与测试环境有所区别(毕竟我们测试只是需要执行测试用例,而不用真的执行一个请求再由对应的Controller来响应),但我们又需要加载ThinkPHP的上下文,如:配置、类加载器、核心类库等。所以如果直接ThinkPHP的统一入口文件,将会提示:无法加载类控制器。
在此, 对Think这个启动类进行了一些修改,即把App::run();注释掉。
发现的问题
ThinkPHP_3.2.2_Full_Log/test_ThinkPHP$ phpunit ./Think_Log_Test.php
通过执行测试用例,很容易发现存在的问题。
There were 4 failures:
- PhpUnderControl_Think_Log_Test::testInitIllegal
/mnt/hgfs/Desktop/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP/Think_Log_Test.php:68
- PhpUnderControl_Think_Log_Test::testSaveWithOtherTypeAfterInit
Failed asserting that file "/mnt/hgfs/Desktop/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP/_test_log.txt" does not exist.
/mnt/hgfs/Desktop/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP/Think_Log_Test.php:132
- PhpUnderControl_Think_Log_Test::testSaveAgainWithOtherType
Failed asserting that file "/mnt/hgfs/Desktop/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP/_test_log_again.txt" does not exist.
/mnt/hgfs/Desktop/ThinkPHP_3.2.2_Full_Log/test_ThinkPHP/Think_Log_Test.php:166
- PhpUnderControl_Think_Log_Test::testWriteWithTypes
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Mock::write(ERR: Message 30) here
-DbMock::write(ERR: Message 30) here
-Mock::write(ERR: Message 30) here
-DbMock::write(ERR: Message 30) here
-'
+''
下面分别就各个问题进行说明。PS:为了快速便于问题及其专题,测试时已经尽力通过测试函数名来体现。
第一个问题:当创建一个不存在的存储代理时,会报Class不存错误。
1 /** 2 * 建议抛出异常,而不是错误 3 * @group testInit 4 */ 5 public function testInitIllegal() 6 { 7 $config = array('type' => 'None'); 8 9 //$rs = Think\Log::init($config); 10 $this->fail(); 11 }
严格来说,这不是一个潜在的问题,但由于存储代理的类型是外部传入指定的,可能在应用项目业务进行设置时会误传了不存在的Driver,结果导致了项目崩溃。估计这显然不是开发者希望看到,毕竟因为不能写入一条Log(甚至只是初始化,还没开始用Log)就导致了核心的业务流程挂掉会让人比较难接受。在此故意将其置为失败,是为了提醒后面要对其进行改进。
第二个问题:Log组件初始化后,改用其他类型存储无效
1 /** 2 * @group testSave 3 */ 4 public function testSaveWithOtherTypeAfterInit() 5 { 6 Think\Log::init(array('type' => 'File')); 7 8 $recordData = $this->provideRecordData(); 9 foreach ($recordData as $data) { 10 Think\Log::record($data[0], $data[1], $data[2]); 11 } 12 13 $type = 'Mock'; 14 $destination = dirname(__FILE__) . '/_test_log.txt'; 15 @unlink($destination); 16 17 Think\Log::save($type, $destination); 18 19 $this->assertFileNotExists($destination); 20 }
其中,Mock类是一个测试模拟类,用来模拟Log存储的写操作,实际上是将其echo输出。
显然,这里的用户期望是,当我切换其他存储方式时,LOG组件应该可以相应切换过去,不然Log在save操作时提供type这个参数就无意义了。而且,相信实际开发中也确实会存在这样的用户(这里的用户指开发人员)需求。如一些常规性的日记写在文件系统,对于一些级别较高的如支付失败则会将Log存储在邮件、内存或者其他地方,以便监控告警。
然而,一旦对Log进行了初始化,再调用save时却达不到预期的效果。也许会导致某些用户因抱怨收不到日记而觉得这个框架不够人性化。
其实是可以做到的,只需要再次调用 Log::init()就可以进行存储切换,但需要换回来时,又需要再次调用 init()。显然,还是过于麻烦。
第三个问题:(类似第二个问题)再次保存日记并改用其他存储类型时无效
这个问题类似于第二个问题,但在使用上又区别于第二问题。主要是第二个问题是初始化改变,这次是写入后(内部隐藏进行了初始化),改用其他类型写入后,却依然会写到第一个存储类型去。这里引伸另一个问题,即在record后,如果用户需要将这一次的日记写在多个不同的存储位置时也是做不到的,因为框架在每次写入时都会将内存中的日记清空。
第四个问题:(类似第二个问题)用不同类型的存储方式写入时无效
1 public function testWriteWithTypes() 2 { 3 $message = 'Message 30'; 4 $level = Think\Log::ERR; 5 6 Think\Log::write($message, $level, 'Mock'); 7 8 Think\Log::write($message, $level, 'DbMock'); 9 10 Think\Log::write($message, $level, 'Mock'); 11 12 Think\Log::write($message, $level, 'DbMock'); 13 $this->expectOutputString("Mock::write(ERR: Message 30) here 14DbMock::write(ERR: Message 30) here 15Mock::write(ERR: Message 30) here 16DbMock::write(ERR: Message 30) here 17");
此调用其实更多是用于在底层,而不是在业务上。所以个人觉得不应该将此接口开放给外部,从而减少用户误用。为了测试不同的底层写入,这里添加了另一个DB模拟存储。
问题修改与重构
上述的问题2-4,其实都是由于Log类里面对存储代理管理方式上。查看源码会发现对于Log::$storage存在多次初始化。如在init()中是强制重新创建,而在save和write时是没初始化时才人创建。这样设计其实就已经给用户带来了混淆:我在使用Log时需要不需要对其进行初始化呢,如果不初始化会有什么影响吗?而且存在不一致的情况,如在init时,会默认使用File,而在其他地方则会使用配置中的LOG_TYPE,这其实也会给用户带来困惑:为什么我设置的LOG存储类型不生效!这时别人可能会对这位困惑的开发人没说:嘿,伙计!你不应该初始化的,因为这样会让你配置的值无效。为什么?很显然,这不是正常人的思维。
为了修复这个问题,我们还是使用刚才那个测试,不过稍微有所改动。当然,对于一个框架,其推广程度更多来源于用户对其的信任,而其信任更多来自于框架自身的向前兼容性。所以在进行问题修改与重构时,需要注意到这一点。
为了区分和不影响原来的Log,在此我加了新的LogV2,表示第二版。
1 protected static function _getStorage($type = '') 2 { 3 $type = $type?:C('LOG_TYPE'); 4 5 if (!isset(self::$storage[$type])) { 6 self::$storage[$type] = self::_createStorage($type); 7 } 8 9 return self::$storage[$type]; 10 } 11 12 protected static function _createStorage($type = '') 13 { 14 $class = 'Think\\Log\\Driver\\'. ucwords($type); 15 16 if (!class_exists($class)) { 17 throw new Exception("Class $class not found when try to create storage"); 18 } 19 20 $writter = new $class(); 21 22 if (is_callable($writter, 'write')) { 23 throw new Exception("Class $class should implements interface Writter"); 24 } 25 26 return $writter; 27 }
很多时候修复一个问题不在于这个问题有多难,而是相关的代码分得太散。导致这种情况发生很可能是要么代码重复了,要么耦合性太高。对于Log中存储类型不能切换的问题就在于创建存储代理的代码重复出现。所以,首先就应对这块代码进行了封装。这里也修复了不能使用配置值的问题。同时为了满足多个不同存储类型间的切换,存储代理也保存了多份,以便来回切换。
为了规范存储类型的行为,这里加了一个Writter的接口,以便各Driver实现。
1<?php 2namespace Think\Log\Driver; 3 4interface Writter 5{ 6 public function write($log,$destination=''); 7}
运行一下测试用例,一切正常。
1ThinkPHP_3.2.2_Full_Log/test_ThinkPHP$ phpunit ./Think_LogV2_Test.php 2PHPUnit 4.0.15 by Sebastian Bergmann. 3 4...........Mock::write(ALERT: LogV2 message 1 5CRIT: LogV2 message 2 6DEBUG: LogV2 message 3 7ERR: LogV2 message 4 8WARN: LogV2 message 5) here 9... 10 11Time: 57 ms, Memory: 4.50Mb 12 13OK (14 tests, 7 assertions)
开发式设计还是减少用户关注点?
很多人都是矛盾的,比如说我们都希望别人去遵守交通规则,却想自己有自由,可以随意闯红灯。对于框架设计也是。有人说框架其实就是“不能让你做什么”。如日记、会话、安全等这些公共功能框架都已提供,不需要再实现,直接使用即可。用户在接触到一个框架时也希望其能尽快上手,减少学习成本。但项目的需求是多样的,开发用户的需求也是多样的,他们也有着非功能性的要求。
如上面的LOG在每次写入日记 后都清空内存纪录,并且强制在拼接多条日记时使用了“\r\n”分割符,可能设计者当初只想用于在Windows下的文件纪录,但可能实际上用户的日记保存介质和格式是多样的,可能是数据库,可能是Redis,可能是HTML,也可能是Excel,甚至可能是短信。所以这也就限制了用户保存的次数和保存的最终位置。
个人觉得,应该将上面这些业务需求开放出来,改由用户去定义或者去限制。
对于用户关注点上,Log的设计对外的接口有iint(), record(), save(), write(),这四个,隐藏着构造函数。很显示对于日记组件,用户在规约上只需要写日记。但发现有4!个组合方式,难免会让用户对多种组合方式的使用存在误解。同时,创建一个Log的对象其实也是没什么意义,反而会误导用户我既然可以New又何须使用init()?无形中又多了一种误用的方式。
所以应该将构造函数设为保护的,并且简化Log的接口。
更多建议
//TODO
最终完整的LogV2 (部分注释已去掉,方便查看)
1<?php 2namespace Think; 3/** 4 * 日志处理类 5 */ 6class LogV2 { 7 8 // 日志级别 从上到下,由低到高 9 const EMERG = 'EMERG'; // 严重错误: 导致系统崩溃无法使用 10 const ALERT = 'ALERT'; // 警戒性错误: 必须被立即修改的错误 11 const CRIT = 'CRIT'; // 临界值错误: 超过临界值的错误,例如一天24小时,而输入的是25小时这样 12 const ERR = 'ERR'; // 一般错误: 一般性错误 13 const WARN = 'WARN'; // 警告性错误: 需要发出警告的错误 14 const NOTICE = 'NOTIC'; // 通知: 程序可以运行但是还不够完美的错误 15 const INFO = 'INFO'; // 信息: 程序输出信息 16 const DEBUG = 'DEBUG'; // 调试: 调试信息 17 const SQL = 'SQL'; // SQL:SQL语句 注意只在调试模式开启时有效 18 19 // 日志信息 20 static protected $log = array(); 21 22 // 日志存储 23 static protected $storage = array(); 24 25 private function __construct() 26 { 27 } 28 29 // 日志初始化 30 static public function init($config=array()){ 31 $type = isset($config['type']) ? $config['type'] : ''; 32 self::_getStorage($type); 33 } 34 35 static function record($message,$level=self::ERR,$record=false) { 36 if($record || false !== strpos(C('LOG_LEVEL'),$level)) { 37 self::$log[] = "{$level}: {$message}"; 38 } 39 } 40 41 static function save($type='',$destination='') { 42 if(empty(self::$log)) { 43 return ; 44 } 45 46 if(empty($destination)) { 47 $destination = C('LOG_PATH').date('y_m_d').'.log'; 48 } 49 50 $message = implode("\r\n", self::$log); 51 52 self::_getStorage($type)->write($message,$destination); 53 // 保存后清空日志缓存 54 self::$log = array(); 55 } 56 57 static function write($message,$level=self::ERR,$type='',$destination='') { 58 $destination = $destination ? :C('LOG_PATH').date('y_m_d').'.log'; 59 self::_getStorage($type)->write("{$level}: {$message}", $destination); 60 } 61 62 protected static function _getStorage($type = '') 63 { 64 $type = $type?:C('LOG_TYPE'); 65 66 if (!isset(self::$storage[$type])) { 67 self::$storage[$type] = self::_createStorage($type); 68 } 69 70 return self::$storage[$type]; 71 } 72 73 protected static function _createStorage($type = '') 74 { 75 $class = 'Think\\Log\\Driver\\'. ucwords($type); 76 77 if (!class_exists($class)) { 78 throw new Exception("Class $class not found when try to create storage"); 79 } 80 81 $writter = new $class(); 82 83 if (is_callable($writter, 'write')) { 84 throw new Exception("Class $class should implements interface Writter"); 85 } 86 87 return $writter; 88 } 89}