Some improvements in tests #4:#13408
Some improvements in tests #4:#13408wilsonge merged 4 commits intojoomla:stagingfrom frankmayer:improvements-in-tests-4
Conversation
- removal of some unused variables (and inserted todo's if unclear) - removal of null assignments on class properties;
| * @return boolean True on success, false otherwise | ||
| * | ||
| * @since 12.1 | ||
| */ |
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.
| '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.
| */ | ||
| 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