Shared NPM Notes

[Westbrook]

I'm working through getting the coverage running for an NPM build, and wanted to drop some of my notes as I work through various things in case anyone else runs into them.

1) It looks like the current approach to capturing the absolutePath is a little presumptive:

var absolutePath = req.url.replace(/^\/[^/]+\/[^/]+/, root);

If you're using a package name like @org/package there ends up being three levels of folder path to remove instead of two, so I think it might be better to use something like:

var re = new RegExp(`^\/[^/]+\/${basename.replace('/','\/')}`);
var absolutePath = req.url.replace(re, root);

2) I've just gotten this far and have to pivot to something else for a while, but once you're able to get files into the coverage checker I'm getting:

Failed to parse file: /Users/westbrook/Documents/web/repos/ll-footer/ll-footer.js
Error: Line 1: Unexpected token
    at ErrorHandler.constructError (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:5004:22)
    at ErrorHandler.createError (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:5020:27)
    at Parser.unexpectedTokenError (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:1985:39)
    at Parser.tolerateUnexpectedToken (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:1998:42)
    at Parser.parseStatementListItem (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:3361:31)
    at Parser.parseFunctionSourceElements (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:4183:29)
    at Parser.parseFunctionExpression (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:4396:26)
    at Parser.parsePrimaryExpression (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:2373:38)
    at Parser.inheritCoverGrammar (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:2278:37)
    at Parser.parseLeftHandSideExpressionAllowCall (/Users/westbrook/Documents/web/repos/ll-footer/node_modules/esprima/dist/esprima.js:2892:26)

Seems fairly likely that this is due to the import {} from ... statement, but I'll have to get into it later. First thought is either esprima isn't up-to-date on the import scene, being their last release was a year ago, or that the settings being pushed into it aren't.

More soon.

[Westbrook]

When running without wct-istanbub I get a solid test pass, i.e. chrome 66 (4/0/0) however, when I include "istanbub" in the wct.conf.json I get chrome 66 () unless I make "include" an empty array, i.e. "include": [], which means no coverage report...

I've hit a wall moving things about and debugging to try to explain this better, but it seems like there might be something with the load order/timing going on. I can also get an equally distressing result where it runs the tests but it fails them because the component code doesn't seem to be loaded at test time, which is where I begin to get that from, but that's even harder to explain.

[Bubbit]

I did some digging today, it's due to the instrumenter, which seems to break relative imports.

Errors like: Error: Failed to resolve module specifier "@polymer/polymer/polymer-element.js". Relative references must start with either "/", "./", or "../". so somewhere a long the way the injected coverage code breaks the imports.

If I copy-paste the code that the instrumenter generates and try to inspect that with polymer-serve there are no issues, so must be something related to WCT

I'll keep digging :)

[Bubbit]

Just released version 0.2.0-pre.2, I got it working with the starter-kit of polymer 3, I did have to overwrite the includes for @polymer/polymer/polymer-element.js to /node_modules/etc, so I still have to fix that.

[Westbrook]

Finally got a chance to look at the new version, and while it no longer fails outright, I'm still getting much of the issues I was before.

• To get files to be "included" be sure that the mini-match is explicit and written with a proceeding '/':

  "include": ["/ll-option.js"],

  It seems you chose to add the basepath to the whitelist, rather than remove it from the req.url, a la var whitelist = options.include.map(x => basepath + x); which makes sense. It's hard to say which is more brittle, but this approach probably should either test for or document the presence/absence of a /. The current code expects, it could be x =>${basepath}/${x}instead or something likex => ${basepath}${x[0]==='/'?'':'/'}${x} or, you know, better!

• Once files are actually read as deserving of "instrumentation", the magic node pathing seems to be interrupted:

  Failed to resolve module specifier "@polymer/lit-element/lit-element.js". Relative references must start with either "/", "./", or "../".

  This part is well beyond my understanding of the new NPM based build/serve code yet, but I'll keep looking.

[Bubbit]

I've published a new version (still very hacky, didn't test it with polymer 2 yet, only 3 ^^)

I did not solve the include part yet, was focused on the scoped package loading and that works now \o/.

I'm going to start to add some tests, clean up the code etc before I make it a real release, but if you have time give it a spin as I only tested it with a small code base :)

[Westbrook]

🍾 This is definitely looking better!!

I'll need to look into it a little more, but anecdotally a good test to add is confirmation that none of the transforms/etc are messing with the line number reporting in the coverage chat. I'm experiencing a couple of results pointing to comments, but it may just be early here still.

I've got a number of components I'm converting right now, so I'll get able to test some more in the next couple of days.

Very exciting 🙌

[Bubbit]

Released pre.4 with some tests and a fix for the include/exclude paths.

I couldn't find a good example for the line number reporting issue you mention, do you have an example somewhere I could use/see?

[Westbrook]

The more I look at this, the more I think it way have just been my ego saying the lines were off. I've worked through adding ~10% extra coverage to my statements and branches, and I while I've disagreed with those lines not being covered, actual testing has caused them to be covered. So, I think it's not the problem I thought it was.

[Westbrook]

One question, have you seen anywhere there being a minimum line count or file size for inclusion in the instrumenting? I'm getting a very small file (28 lines, 696 bytes) get left out of the instrumented list, even after it being marked with "coverage instrument" when using polymer test --verbose. This may have been a thing before, but I never noticed it.

[Westbrook]

I’m expanding to some more repos and so far it might not be the file size but simple the number of files. I’ve gotten each new repo to list only three files in the instrumentation, no matter how many there should be. Not sure what’s going on there, but I’ll follow the rabbit a little more and see if I can get any info for you.

[Bubbit]

With my polymer 2 project that I use at work I still get all the files covered, I'll have to run it with a bigger Polymer 3 project to check tho :)

EDIT: Just updated the starter kit with some more tests and for some reason I only get 3 files instead of the expected 6, will have a deep-dive

EDIT2: For some reason the last suite does not return any coverage data, on sub-suite-end it's just empty for some reason

[Bubbit]

I just pushed an update (not a release yet, istanbuljs branch) for covering all the files (solved the issue in my small test prject) can you give it a shot with your projects?

[Westbrook]

First test is 🎉 !

I'm gonna swing back to this later today and put a couple of other repos to the test, but that's is looking very good!