If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Update the test runner

RESOLVED FIXED in 1.0

Status

L20n
JS Library
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

unspecified
x86_64
Linux

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Our test suite got very outdated.  Let's fix this so that writing tests is easy and rewarding.
(Assignee)

Updated

4 years ago
Assignee: nobody → stas
Priority: -- → P1
Target Milestone: --- → 1.0
(Assignee)

Comment 1

4 years ago
Created attachment 759828 [details] [diff] [review]
The test runner works now (55 tests for starters)

I cleaned up a lot of things and made others easier.  A few random (well, not so random) notes:

- I removed the tests/fixtures submodule;  I haven't removed the lol-fixtures repo yet, but I want to eventually, when I moved all files from it;  this might create one-off problems when merging

- I propose the following dir layout:

  - tests/
    - lib - unit tests for lib/;  if there are many (like compiler or parser) put them in a folder
      - client - tests requiring a browser to be run
    - integration - integration tests for the whole library
    - bindings - unit tests for bindings

- unit tests should prefer L20n code inlined in the test file (thanks to this the testcases double as documentation);  integration tests can use fixtures

- run make test to run all tests

- run make watch to run tests when a file changes (+ growl notifications!)

- run make coverage and open dist/docs/coverage.html to see how well the existing test cases cover the code (not representative, but helpful)

- run make docs to extract all comments and create nice-looking documentation in dist/docs

- I moved docs to dist/docs so that we don't have to worry about keeping them up-to-date in the repo.  I still want to publish them at gh-pages, but I haven't worked on that part yet

- you might need to run npm install to get mocha, docco and jscoverage

- I removed old context tests, they were too outdated;  it would be good to reuse the fixtures from the scenarios that we had for them

- I left old compiler test cases;  the changes to the Makefile make sure they're not run;  I ported errors.js to the new test runner and I will continue porting other test cases as well

- I didn't touch tests/bindings/html which use Jasmine;  we should figure out the story for client-side testing later, in another bug.

- the patch also makes a small change to Context::freeze to fix an issue which I discovered when writing the sample tests

- the patch has 55 passing tests right now;  a modest start, but still :)
Attachment #759828 - Flags: review?(gandalf)
(Assignee)

Comment 2

4 years ago
Even if the porting to the new test runner is not complete or finalized, I'd like to land this patch asap so that we can start writing tests to 1.0 bugs next week.
When I applied the patch and run "make test" I got an error: http://pastebin.mozilla.org/2497348
(Assignee)

Comment 4

4 years ago
Created attachment 760092 [details] [diff] [review]
Patch with -M (to make renames work)

(In reply to Staś Małolepszy :stas from comment #1)
> - the patch also makes a small change to Context::freeze to fix an issue
> which I discovered when writing the sample tests

I removed this from the patch and filed bug 880948.

(In reply to Zbigniew Braniecki [:gandalf] from comment #3)
> When I applied the patch and run "make test" I got an error:
> http://pastebin.mozilla.org/2497348

Hmm, interesting.  I can reproduce this on a newly cloned repo.  I think there's a problem in how git marks renames.  Try this new patch, created with 'git diff -M'.  After you apply it, the following two directories should exist:

  tests/integration
  tests/lib/compiler

If you're still having problems, try checking out my branch at

  https://github.com/stasm/l20n.js/tree/880727-test-runner
Attachment #759828 - Attachment is obsolete: true
Attachment #759828 - Flags: review?(gandalf)
Attachment #760092 - Flags: review?(gandalf)
Attachment #760092 - Flags: review?(gandalf) → review+
(Assignee)

Comment 5

4 years ago
https://github.com/l20n/l20n.js/commit/d9856360e4816dcc450eed13a393d2b698593477

Woo hoo!
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Blocks: 881646
You need to log in before you can comment on or make changes to this bug.