Config::getFromLocalConfig + tests #7639
Conversation
| $globalFile = PIWIK_INCLUDE_PATH . '/tests/resources/Config/global.ini.php'; | ||
| $commonFile = PIWIK_INCLUDE_PATH . '/tests/resources/Config/common.config.ini.php'; | ||
|
|
||
| $config = Config::getInstance(); |
There was a problem hiding this comment.
The singleton instance shouldn't be used in unit tests; create a new instance instead, ie, new Config(...). setTestEnvironment shouldn't be used for these tests as well.
There was a problem hiding this comment.
@diosmosis why are those are used in the other tests? https://github.com/ThaDafinser/piwik/blob/patch-3/tests/PHPUnit/Unit/ConfigTest.php#L52-L54
i updated the tests :-)
There was a problem hiding this comment.
why are those are used in the other tests?
Legacy code :) I've actually removed them in a branch I'm working on.
mattab
added
the
Pull Request WIP
mattab
added
Task
|
@diosmosis @mattab now it's green and the unitTest is changed |
mattab
added
Needs Review
| $config->reload(); | ||
|
|
||
| $configCategory = $config->getFromCommonConfig('Category'); | ||
| $this->assertEquals('valueCommon', $configCategory['key2']); |
There was a problem hiding this comment.
Can you test against the whole section instead of just 'key2'? Ie, array('key2' => ..., 'key3' => ...). Same for the other two tests. After that, I'll merge.
There was a problem hiding this comment.
@diosmosis i added the test.
But as u can see in the previous commit, this test would fail...we should fix that with next config update.
$this->assertEquals('${@piwik(crash))}', $configCategory['key3']);There was a problem hiding this comment.
Dollar signs are supposed to be HTML encoded in config files, this fixes a bug/"feature" in parse_ini_file().
Can you make it test against an array? ie $this->assertEquals(array('key2' => ..., 'key3' => ...), $configCategory);? This will test that the section only has the expected keys, in addition to testing the values of those keys.
There was a problem hiding this comment.
Ah, I see what you mean about the test failure; I'll look into it, it may be a bug.
There was a problem hiding this comment.
@diosmosis i will provide a test against an array, but next time i wont....
Please dont provide information about what you want piece by piece...tell it me one time what you all need or want in the PR to be updated.
The problem is following:
Both (you and me) need to get back everytime again on the PR which needs a lot of unnecessary time and like to complete things
//edit: sry that i'm so directy, but for such a small PR 3 times back is to much
There was a problem hiding this comment.
@diosmosis about the failure with the HTML this need to be checked in another PR, since this was not introduced here.
There was a problem hiding this comment.
Sorry for the back & forth. I don't mind making the tiny changes myself, but I thought it might be rude to do so for someone else's PR. I also don't mind directness, it tends to speed things up.
Re the test failure: I meant I will fix the issue in master before merging this PR.
There was a problem hiding this comment.
👍 for me its not rude (since OS is all about collaboration), but some people may be offended you are right.
|
@diosmosis test is now against the array as well |
Add Config::getFromLocalConfig method to get config section from local config w/o default values from global/common config added + tests.
|
Thanks for merge |
Follow up from #7318