Closed danon closed 3 years ago
@Danon there is something wrong with the build. Not sure if it is caused by these changes. Can you take a look?
@bbankowski I don't think it has anything to do with my changes in ConfigTest.php
.
It was with travis, running on trusty
, and that travis distro didn't have php 7.4. I changed it to default distro and added service: mysql
entry in .travis.yml
, builds now. (afraid it still fails on PHP 8.0 nightly
). I expect this branch being merged to ouzo master
will probably fix master build.
Merged, thanks!
Tests would fail on windows because of
/tmp
usage, but it turns out it's not even necessary.Tests would save
SampleConfigFile
class in/tmp/SampleConfigFile.php
, because there once wasCustomConfig
that could load files, so to test that/tmp/SampleConfigFile.php
was created for testing. Later, it was removed in favour ofSampleConfig
, but instead of removing/tmp/SampleConfigFile.php
, it was left as it was, and then immediatelyinclude_once
. Obviously this is unnecessary since the file isn't tested, only the class that's passed toregisterConfig()
. Without the file, the test doesn't need to operate on filesystem, so tests pass on windows.Tests were reliant on the order and the fact that multiple tests are run. If you run any test that uses
registerConfig()
but notreload()
(soshouldReadMultipleSampleConfigs()
,shouldReadSampleConfig()
,shouldReturnConfigValue()
), they would fail if you run them separately.Further more, because the config remembers the state between tests (
static
field), the tests are sometimes false-positive, so even if you remove the code from given/when it would pass, because other tests registered the config. They would be caught, if the tested values were different, butSampleConfig
always returned the same value for each test, so each assertion passed.To fix it, I made
new SampleConfig()
intonew SampleConfig(['foo' => 'bar'])
, so each test recieves uniquekey=>value
pair, and isn't prone to false positives from other configs.Feel free to squash the commits while merging the PR.