Respec: Add code coverage support

[marcoscaceres]

We should add a code quality tool like code climate to get an idea of gaps in our tests.

[CodHeK]

@marcoscaceres this is the one right ?

[marcoscaceres]

yep, but it's one of many.

[CodHeK]

Codacy is already being used as a code quality tool right ?

[marcoscaceres]

yes, but Codacy doesn't tell us what is being tested by our test suite... can it?

just talking to a friend of mine, she is telling me https://jestjs.io might be compatible with our setup - as our test suite is based on Jasmine.

[CodHeK]

hm..oh, yes! This looks nice, maybe we can try this!

[marcoscaceres]

yeah, I think it's a good option... they should have a "migrating from Jasmine" guide... but please see if there are some blog posts with criticisms from other projects.

[CodHeK]

Uh, yeah using jestjs would make us shift entirely from jasmine to jest right, the tests need to be re-written ?

[marcoscaceres]

@saschanaz, from other projects you work on, any experience or suggestions in this area? I can also ask folks on twitter... though it might digress.

We could check what big open source projects are using... like Babel, or some others.

[CodHeK]

@marcoscaceres got this blog, https://fuhton.com/Migrating-from-Jasmine-to-Jest/ I believe we must shift our testing suite to jest, it's got an inbuilt code-coverage tool and syntax looks pretty similar to jasmine.

[marcoscaceres]

Looks like the APIs are very close... so looks like minimal rewrite. That's a huge bonus.

[CodHeK]

Yes, exactly!

[marcoscaceres]

Some reading material
https://medium.com/welldone-software/an-overview-of-javascript-testing-in-2018-f68950900bc3

[CodHeK]

@marcoscaceres so, how should I proceed towards implementing this ? I skimmed through the jest docs, doesn't look any different than jasmine, should I begin with it's integration then ?

[marcoscaceres]

Not yet. An important part of decision if to weight the pros/cons. It’s why I asked if you could find some blogs criticizing it so we can evaluate its merits.

The other things we would do is maybe prototype one module to see if it works and the report we get.

[CodHeK]

Right! I'll do a bit more research on this first and get back to you!

[CodHeK]

@marcoscaceres we could probably use this https://github.com/skovhus/jest-codemods , might make the jasmine to jest migration really easy!

Also, I couldn't find any criticism towards usage of jest, instead what I observed is that most of them migrate from other testing frameworks into jest as time passes.

[marcoscaceres]

@CodHeK, could you try a couple of our tests, see what comes out? Intrigued to see this working.

[CodHeK]

Using the codemods right ?

[marcoscaceres]

Yep, using codemods.

[CodHeK]

@marcoscaceres I am a little confused here, I see there is karma , mocha jasmine all three of them present in the package.json file, how exactly are we testing here ?

[CodHeK]

@marcoscaceres I am having problems ( permission errors ) with installing jest, is there any sort of restriction in the repo that doesn't let others install new packages ?
Since, I get no errors when I run the same npm install --save-dev jest on other node projects of mine.

[marcoscaceres]

No, there should be no restriction. Can you paste in the error?

[CodHeK]

@marcoscaceres I don't know it says permission denied, I even tried adding sudo didn't work
I tried it on one of my other projects it got installed without any errors! 😕

[CodHeK]

Also, I tried the codemods on the /tests/spec/core/linter-rules folder worked really well! 😄 now I just need to install jest to check if those tests pass.

[marcoscaceres]

I would try removing the entire node_modules directory and doing npm install again. It’s definitely looking like the jest directory has bad permissions for whatever reason.

[CodHeK]

@marcoscaceres it's starting to work! :D

I used codemods on the /tests/spec/core/linter-rules folder, ran npm test got this...

Iit seems jest doesn't work with RequireJS need to find a workaround for this!

[marcoscaceres]

Ok, this is good to know. Additional motivation to get rid of RequireJS as a dependency. Appreciate you digging into this.

[CodHeK]

require(['core/linter-rules/${ruleName}'], ({ rule }) => resolve(rule)); is this the part where RequireJS is being used ?

[marcoscaceres]

Yeah, it could be... what happens if you treat that as an ES6 import instead? The RequestJS is irrelevant.

import { rule } from  "/src/core/linter-rules/check-charset.js";

[CodHeK]

@marcoscaceres import { rule } from '../../../../src/core/linter-rules/check-charset.js'; this worked "
😛

But I get this weird error in the comments section!

[CodHeK]

@marcoscaceres jest worked! 🎉

[marcoscaceres]

Ok, awesome! How do we now pull coverage information from it?

[CodHeK]

We can just run npm run coverage for that!

[marcoscaceres]

I've never done it, so what does it produce?

[CodHeK]

I'll just implement it on the linter-rules folder and post a screenshot here!

[marcoscaceres]

Appreciate that. Excited to see where this is going!

[CodHeK]

@marcoscaceres The coverage is of this sort...

As of now, only the linter-rules files were tested hence the coverage for most is in red

We can also save the output in a file, if that's of any use ?

[marcoscaceres]

Very nice!

We probably don’t need to save to a file, but we might still want this to somehow be part of our continuous intergration process. We would generally want 100% coverage for any new code.

[marcoscaceres]

Ok, something like this might work 🤔

https://github.com/nickmerwin/node-coveralls/blob/master/README.md#jest

[CodHeK]

@marcoscaceres yeah!!

    - stage: Run eslint
      script:
        - npm run lint
    - stage: Run headless
      script:
        - npm run test:headless
    - stage: Run Karma unit tests
      script:
        - travis_retry karma start --single-run --reporters mocha karma.conf.js

I'll have to replace the karma units tests with these right or we run both ? 😕

[marcoscaceres]

Yeah, there seems to be good instructions here:
https://medium.com/@ollelauribostr/start-measuring-coverage-with-jest-travis-ci-and-coveralls-1867928aca42

I’d try bundling everything up into a pull request. I’ve enabled coveralls for ReSpec over on coveralls.io... so should see something work hopefully 🤞

[marcoscaceres]

Note we use “develop” instead of “master”.

[CodHeK]

@marcoscaceres I'll make all the tests ready and passing with jest until then! Should I also do the travis integration ?

I'll do these tasks:

1. migrate all tests to jest.
2. add the coverall script and modify travis.yaml

And send a PR ?

[marcoscaceres]

karma units tests with these right or we run both ?

Not sure yet. Karma will probably break I think, so I think it will be jest only.

[CodHeK]

@marcoscaceres there's a small problem with the other tests, the line afterAll(flushIframes) (eg: algorithms-spec.js) , I don't see where the function flushIframes is being imported here... 😕

[marcoscaceres]

About the work items. I’d suggest sending a pull request now. We won’t merge it, but it’s to make sure we can check it’s on the right track. This might turn out to be a lot of work... so go slow. Do the easy ones first and get feedback.

[CodHeK]

Yes! Sending one now!

[marcoscaceres]

It’s imported via https://github.com/w3c/respec/blob/develop/tests/spec/SpecHelper.js

Which may be part of Jasmine. Jest must have an equivalent.

[CodHeK]

Yes, but how is it imported in the test files, I don't see any require or imports ?

[CodHeK]

Oh, okay these are Globals ! 😅

[marcoscaceres]

Heh, yep... sort of. So:
https://github.com/w3c/respec/blob/develop/tests/SpecRunner.html

And there is one more...

[marcoscaceres]

karma injects it too:

https://github.com/w3c/respec/blob/develop/karma.conf.js

[marcoscaceres]

Jest must have some kind of global loader too.

[CodHeK]

I shall send a PR now, linter-rules folder is set! There's a lot work left on the other test file which I shall complete gradually ...

And yes, jest has something called as mockers I guess for such Globals, not very sure yet, I'll have to check...

[CodHeK]

@marcoscaceres sent a PR - https://github.com/w3c/respec/pull/2085

[marcoscaceres]

What we could do is incrementally transition from Karma/Jasmine to jest being explicit about which runner runs what. That will allow us to do one at a time, and hopefully prevent bit-rot. We could start with the linters and then some of the easy ones.

[CodHeK]

Should I add travis too ? like only for the linter-rules folder ?

[marcoscaceres]

Yes, adding TravisCI support would be great.

[elharony]

Hi @marcoscaceres ,

I would like to contribute and help fixing this issue, as it's marked as Good First Issue but it doesn't seem to be so. Am I right? Please provide some guidelines here! 🙏

[saschanaz]

I intend to fix this in #2187, so removing the tag.

Current state there:

tests/test-node.js
  Core — data-abbr
    √ processes definition abbreviations (4500.396ms)
    √ allows different abbreviation combinations to link to dfn (76.59ms)
    √ correctly parses and abbreviates different Abbreviation syntax (80.698ms)
    √ warns when used with unsupported elements (68.492ms)
    √ normalizes title of added abbr for unwanted spaces (65.78ms)


  5 passing (10s)
-----------------------|----------|----------|----------|----------|-------------------|
File                   |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
-----------------------|----------|----------|----------|----------|-------------------|
All files              |    29.92 |     23.2 |    27.75 |    30.43 |                   |
 js                    |    71.43 |    62.16 |    82.35 |    73.26 |                   |
  html-parser.js       |    72.73 |    58.33 |    66.67 |       80 |              5,17 |
  html-template.js     |       70 |       64 |    83.33 |    70.15 |... 09,110,111,113 |
  respec-version.js    |       80 |    58.33 |      100 |    88.89 |                 5 |
 src                   |       90 |    44.44 |      100 |       90 |                   |
  index.js             |      100 |      100 |      100 |      100 |                   |
  respec-document.js   |    83.33 |    44.44 |      100 |    83.33 |             14,30 |
 src/core              |    21.47 |    15.15 |    20.49 |    21.82 |                   |
  asset-loader.js      |      100 |      100 |      100 |      100 |                   |
  cgbg-headers.js      |        0 |        0 |        0 |        0 |                 7 |
  cgbg-sotd.js         |        0 |        0 |        0 |        0 |                 5 |
  headers.js           |    66.67 |    44.05 |      100 |    66.67 |... 59,277,286,292 |
  show-link.js         |        0 |        0 |        0 |        0 | 6,8,9,10,11,13,20 |
  show-logo.js         |        0 |        0 |        0 |        0 |... 11,14,24,25,26 |
  show-people.js       |    45.45 |    30.43 |       50 |    46.88 |... 99,102,104,105 |
  sotd.js              |    45.83 |    36.14 |    41.67 |    45.83 |... 80,283,310,337 |
 tests                 |       75 |      100 |       50 |       75 |                   |
  test-node.js         |       75 |      100 |       50 |       75 |             27,28 |
 tests/spec            |    12.79 |       10 |       15 |    12.79 |                   |
  SpecHelper.js        |    12.79 |       10 |       15 |    12.79 |... 85,199,210,217 |
 tests/spec/core       |      100 |      100 |      100 |      100 |                   |
  data-abbr-spec.js    |      100 |      100 |      100 |      100 |                   |
-----------------------|----------|----------|----------|----------|-------------------|

[marcoscaceres]

@saschanaz, looks like you had coverage kinda working in #2187... is it possible to extract just that from #2187?

[saschanaz]

@marcoscaceres

1. It works on Node.js, which we don't prioritize now 😱
2. I used another test framework to do that, migration was not very straightforward.

[marcoscaceres]

No problem. I’ll see if there is some other alternative.