FirstLegoLeague / fllscoring

FLL Scoring app. See Demo:
http://firstlegoleague.github.io/fllscoring/
GNU General Public License v2.0
14 stars 19 forks source link

Fix test dependencies #293

Closed poelstra closed 6 years ago

poelstra commented 6 years ago

It appeared (while working on #281 and on refactoring ranking out to common code) that something is fairly broken with the way the factory() function is used in our tests.

The original idea was to replace RequireJS while running the tests, in order to allow passing e.g. mocks as dependencies for the modules. (Note that Angular itself provides a similar mechanism, too.)

The way factory() worked, though, is that it cached the first invocation, which means that any later invocations would get that module with a potentially different set of dependencies. These could be mocked dependencies, or could even be missing, because there was no check for such missing dependencies.

Specifically, e.g. adding a dependency in e.g. ng-scores worked fine in 'real life', but fails mysteriously in tests. This appears to be caused because another tests already instantiated ng-scores, but failed to pass this new reference, and that instance was then cached.

This PR fixes this by making any call to factory return a new 'eco system' of modules (all of them are re-instantiated). However, because there is only one instance of angular, any angular modules defined as such will overwrite each other, so the calls to factory() must directly precede the actual tests. Hence the move to beforeEach().

As a side-effect, a missing dependency in ng-scores was found, which is now fixed.

Note that modules that use the $fs service are changed to have a dummy-module for services/fs, which is fine in that case, because the mocked $fs doesn't use services/fs.

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.1%) to 87.637% when pulling 045005cb72aa3771b661cb3d5d06ba6d6760b235 on fix_test_dependencies into 303fb8f7deb5c52397ae43ce394adba792cdee85 on master.

kmeesters commented 6 years ago

Have tested this on Win10 & Node 8.5.0 and works fine (all tests pass) but I feel I don't have enough expertise to asses the implications code-wise. @rikkertkoppes ?

rikkertkoppes commented 6 years ago

Implications are only in tests, so rather limited. It looks like a solid improvement.

For the longer term however, I would opt to completely remove requirejs and use webpack or something instead

poelstra commented 6 years ago

Thanks Rikkert. Fully agree with replacing with webpack or so some day. Not for now, though :)

@kmeesters Please wait with merging.

I'm adding a bit of functionality to support common (client + server) code. Also adding some tests for the stuff I already did, plus these new things.

poelstra commented 6 years ago

@kmeesters Done, the tests should make it more clear what the intention of the changes are, too. Feel free to merge :)

coveralls commented 6 years ago

Coverage Status

Coverage increased (+0.1%) to 87.637% when pulling 8a447ced2b2dacdcde49578049ab6ff2e101b43d on fix_test_dependencies into 303fb8f7deb5c52397ae43ce394adba792cdee85 on master.

kmeesters commented 6 years ago

Test (Firefox 49.0.0 (Windows 10 0.0.0): Executed 298 of 314 (skipped 16) SUCCESS (1.888 secs / 1.528 secs)) Node 8.5.0