Some improvements in tests #4: #13408
Conversation
- removal of some unused variables (and inserted todo's if unclear) - removal of null assignments on class properties;
| * | ||
| * @since 12.1 | ||
| */ | ||
| public static function isSupported() |
There was a problem hiding this comment.
All of this classes subclasses inherit this method. It needs to be kept I'm pretty sure?
There was a problem hiding this comment.
i add that doubt also, but doesn't that came from JCacheStorage class (which this one extends)?
There was a problem hiding this comment.
@wilsonge I noticed that it extends JCacheStorage, which has the identical function. Am I missing something?
There was a problem hiding this comment.
This one should be fine. It's a mock for JCacheStorage, not a test case being extended.
| @@ -1201,6 +1201,8 @@ public function testParseXMLLanguageFile() | |||
| 'Line: ' . __LINE__ | |||
| ); | |||
|
|
|||
| //todo: OK to remove this unused assignment and following assertion? Or is some implementation missing? | |||
There was a problem hiding this comment.
You can remove all this. Even if we had the file, assuming it had the correct Spanish data, the test case would fail because the $option array uses English data.
There was a problem hiding this comment.
That was my thinking, too, but I wasn't sure if someone would want to expand and correct this test. Will remove.
| @@ -44,6 +44,8 @@ public function __construct($options = array()) | |||
| */ | |||
| public function get($id, $group, $checkTime = true) | |||
| { | |||
| //todo: OK to remove this unused assignment? Or is some implementation missing? | |||
There was a problem hiding this comment.
In general, it looks like in all cases the generated cache ID isn't being used but the ID passed as a parameter into the methods is. So either fix all of them to use the generated cache ID or just remove the calls.
|
Awesome - Thanks! |
Summary of Changes
Some improvements in tests 4:
Testing Instructions
Code review only.
Documentation Changes Required
None