Set language default correctly, add server unit tests #3367
Conversation
There was a problem hiding this comment.
Looks good except for the name conflict in package.json (test:server).
Per npm-run-all question: we have several kinds of tests, and I am unsure if we should run all of them:
- Unit tests (like what this adds, and there's one other unit test)
- Tests that require the server to run on localhost:10080
- Tests that test the browser (MochiTest, which don't run at all, and the Selenium test)... and the Selenium test needs the server running
- Functional tests for the server (test/server/, but not test/server/unit/)
- Really slow functional tests (test/server/test_file_management.py)
Running unit tests often is easy, and good (though they almost never fail). The browser acceptance test is the best test, so even though it is slow it's still good. Tests that need the server have to be run in the "right" way. Most server tests run fairly quickly, except for the really slow one (which manages the startup and shutdown of a server on its own, rendering it particularly slow).
So... we could name the integration tests differently than unit tests? Then we could use different run-all invocations to get different sets? In the CI tests we just run them as separate lines, which is fine.
package.json
Outdated
| @@ -87,6 +87,7 @@ | |||
| "nsp": "nsp check", | |||
| "posttest": "npm run lint", | |||
| "test": "make bootstrap_zip && mocha test/", | |||
| "test:server": "mocha --recursive test/server/unit/", | |||
There was a problem hiding this comment.
I guess the conflict algorithm isn't catching this, but with the other test-related patches merged I assume this will conflict.
There was a problem hiding this comment.
I'm happy to just back this line out and figure out the details later. I"ll file a followup bug.
There was a problem hiding this comment.
It could just be renamed to test:server_unit or something. Or does mocha test/ test/server/unit/ work?
jaredhirsch
force-pushed
the
3231-english-default
branch
from
5a0c09b to
c6553b2
Compare
(Thought I had goofed up the header mocking in the previous PR, but it seems correct.)
Deliberately making the unit test additions as small as possible here, while still providing diagnostic value.
Open question: should we change npm test to npm-run-all test:*?