JS Unit Tests & JSHint, using Jasmine #5
Conversation
antoviaque
force-pushed
the
js-tests
branch
from
067b216
to
a763067
Compare
| jasmine.sanitizeRestangularAll = function(items) { | ||
| return _.map(items, function(value, index) { | ||
| return jasmine.sanitizeRestangularOne(value); | ||
| }); |
There was a problem hiding this comment.
Nit: This looks like it is equivalent to
return _.map(items, jasmine.sanitizeRestangularOne);
|
@antoviaque Looks good to me, 👍. I had to learn about Angular while doing this review, so some of the comments I added are probably unqualified. :) Can the JS dependencies be installed via bower or npm or something? Including the whole files in the repository feels a bit cumbersome to me. There are outstanding model changes that don't have a migration yet (not introduced with this PR). |
antoviaque
force-pushed
the
js-tests
branch
7 times, most recently
from
89cd612
to
661e878
Compare
Automatically invoked by the make commands that need it. This allows to avoid including the full external source code in our git repo
Enable X11 forwarding, fix DNS resolving under Ubuntu & install Firefox
antoviaque
force-pushed
the
js-tests
branch
from
661e878
to
3563594
Compare
|
@smarnach Thanks for the review! I'm learning Angular too, so my code is also unqualified : ) Thanks for catching these issues. For the JS dependencies, I agree that it isn't optimal to have them in the repository, they pollute diffs & PRs. However, I would like to avoid node/npm dependencies - I've always found the packaging messy, and it makes the application environment more complex. What is possible to do without any additional dependency, though, is to retrieve the files via Makefile rules - I've done that: 0b90a97 . For the outstanding model changes, could that be because of #7 (comment) ? I'll take care of it in that PR, since Matjaz already commented about it there. |
JS Unit Tests & JSHint, using Jasmine
| underscore-min.map | ||
|
|
||
| angular.min.js: | ||
| $(GET) $@ $(CDNJS)/angular.js/1.4.6/angular.min.js |
There was a problem hiding this comment.
These rules have the disadvantage that they won't reload the files when the version was changed in the Makefile. So if upstream bumps the version, you pull and run make <whatever>, you will still have the old versions of the JS files, since make can't detect they are not up to date.
A proper solution to this problem would be to cache the version (or URL) used for each file, and redownload it if the URL changed, but this would require a bit of coding. A simpler solution that doesn't get all cases right would be to use
GET = wget -qN
above, remove $@ in all rules, create a .FORCE target
.PHONY: .FORCE
and add it as a dependency to all rules. Make will run the rules every time now, and wget will compare the timestamp of the local files with the remote file, and redownload it if the remote file is newer (so downgrading will likely fail, while upgrading to newer versions should work correctly).
(You could also use wget -qNO and leave $@ in. However, -N disables the numbered backups anyway, so the output filename will be set correctly automatically.)
There was a problem hiding this comment.
@smarnach It will reload them - the files are removed as part of the make clean rule, which is invoked before any execution. So it should always be up to date:
# This is the 1st commit message: Re-writing circle.yml to store code coverage in CircleCI's workspace and collect them afterwards. Test run parallelization is used with environment variables instead of using custom test runner. # This is the commit message #2: Update Makefile message. # This is the commit message #3: Remove cov.html from cov.combine. # This is the commit message #4: Stop running all tests in all containers # This is the commit message #5: Cleanup
# This is the 1st commit message: Re-writing circle.yml to store code coverage in CircleCI's workspace and collect them afterwards. Test run parallelization is used with environment variables instead of using custom test runner. # This is the commit message #2: Update Makefile message. # This is the commit message #3: Remove cov.html from cov.combine. # This is the commit message #4: Stop running all tests in all containers # This is the commit message #5: Cleanup # This is the commit message #6: Experiment with different IDs for each coverage report # This is the commit message #7: Gathering raw coverage from all steps # This is the commit message #8: Always copy coverage and log message when not found # This is the commit message #9: Log copying raw coverage files for debugging
Adds unit tests for the JS code, and a linter check (JSHint). Run via jasmine in the browser (see
make test_js_weband then go to http://localhost:8888/ ) for debugging, or in a CI manner via selenium andjasmine-ci(seemake test_js).