Bubbit / wct-istanbub

11 stars 5 forks source link

Still getting wrong coverage report #1

Closed Supacoco closed 6 years ago

Supacoco commented 6 years ago

Hey! I saw your PR onweb-component-tester-istanbul and definitly needed to test it :) So, thanks for providing a fix!

Unfortunately I continue having wronged coverage reports. I explicitly installed web-component-tester@6.3.0 and wct-instanbub. I also updated my wct config file to rename instanbul to istanbub.

This is the output I get from my CI:

chrome 62                Tests passed
chrome 62                CALL quit()
chrome 62                DELETE /session/:sessionID
20:42:14.738 INFO - Executing: [delete session: 2efcf891-5aba-461d-9eaa-ecfb11f41d38])
20:42:16.817 INFO - Command failed to close cleanly. Destroying forcefully (v2). [node_modules/selenium-standalone/.selenium/chromedriver/2.27-x64-chromedriver, --port=5579][ {}]
20:42:17.824 ERROR - Unable to kill process with PID 475
20:42:17.825 INFO - Done: [delete session: 2efcf891-5aba-461d-9eaa-ecfb11f41d38]
chrome 62                RESPONSE quit()
chrome 62                BrowserRunner complete
Test run ended with great success

chrome 62 (9/0/0)                     

=============================== Coverage summary ===============================
Statements   : 100% ( 0/0 )
Branches     : 100% ( 0/0 )
Functions    : 100% ( 0/0 )
Lines        : 100% ( 0/0 )
================================================================================

Do I get this result because wct failed to shutdown selenium correctly?

I'll gladly discuss/give some help to find a solution on this.

Bubbit commented 6 years ago

Hi, first of all thanks for testing it out :)

I've tested it myself with this application, https://travis-ci.org/Bubbit/polymerTesting/builds/312879603

You do need to have at least web-component-tester 6.4.x as the fix for the hook was introduced in 6.4.0

Hope this helps, let me know if you still have issues.

Westbrook commented 6 years ago

@Bubbit I would question whether you're getting the expected behavior out of the code at https://github.com/Bubbit/wct-istanbub/blob/master/lib/middleware.js#L58

As applied in your test, this works great. However, that is because you've put all of your component code in a directory. This may be an expected form of file organization for a "project/site", but in the context of a stand alone component, I would expect the code to live in the root, in which case the output of the code at the line referenced above is a little tricky.

For instance I have a component named lingo-live-classroom.html and it lives in the root of my file structure. When requested in the testing process, the following is experienced:

Variable Value
req.url /components/lingo-live-classroom/lingo-live-classroom.html
absolutePath /Users/userName/Documents/web/repos/lingo-live-classroom/lingo-live-classroom.html
relativePath /lingo-live-classroom.html

This in theory would work fine, HOWEVER lets follow that logic for a dependency, let's say polymer.html, where the following is experience:

Variable Value
req.url /components/polymer/polymer.html
absolutePath /Users/userName/Documents/web/repos/lingo-live-classroom/polymer.html
relativePath /polymer.html

This is where I'd say things have gone wrong.

That is clearly not the "absolutePath" of that file. I would expect the correct results to read /Users/userName/Documents/web/repos/lingo-live-classroom/bower_components/polymer/polymer.html, is that incorrect?

And, while I'm not sure what the correct "relativePath" would be in that instance, I think something like bower_components/polymer/polymer.html would be more appropriate, such that it would be caught in the "exclude" blob more easily.

As the code stands, it can be worked around by being very explicit with the "include" entry. For my example, the following performs as expected:

"include": [
  "/lingo-live-classroom.html"
]

As you have more than one file requiring coverage, or a naming convention requiring coverage, this can scale up as such. However, this seems like more overhead for the developer than is appropriate. Do you think there is a sensible way to have these paths register more true to what is going on in the file system so that our include and exclude expectations can be more thoroughly met?

Supacoco commented 6 years ago

Hey!

I've tried with web-component-tester@6.4.1 but still have the same output. I'm currently running polymer@1.10.0. Could it be one of the cause?

Supacoco commented 6 years ago

@Westbrook's comment helped me figure out what was going wrong. As he mentioned, my folder was reflecting a standalone-component file structure.

I tried to reflect your file structure (src/component-name/component-name.html) and it worked fine.

Bubbit commented 6 years ago

@Supacoco On one side that's good, other side means there is room for improvement ^^.

@Westbrook I'll see if I have time before the weekend else I'll make time this weekend to look into this issue :)

Bubbit commented 6 years ago

@Supacoco & @Westbrook Didn't have much time this weekend sadly, but did some last night ^^ but do you see any time to test this new approach and see if it works for your use cases?

Westbrook commented 6 years ago

@Bubbit just got a chance to give this a run and am happy to say that this now gives me the insight into my lack of code coverage in the ways that I would have expected to do so. Thanks for getting into this for us!

Supacoco commented 6 years ago

Sorry for the delay. I had some time to test this fix today. I have no clue on what's happening :(

Tests seem to pass but the coverage triggers an error.

chrome 63                Tests passed
Test run ended with great success

chrome 63 (12/0/0)                    

TypeError: Cannot read property 'start' of undefined

ERROR: Job failed: exit code 1
Bubbit commented 6 years ago

Ah just saw your message due to holiday stuff, will have a look as soon as I can :) Do you have a demo repo somewhere that I can use as a test or all internal stuff?

Supacoco commented 6 years ago

No problem, same stuff here.

Unfortunately, the code is related to corporate stuff. I'll create a new repo with the bare minimum. I'll keep you posted.

Thanks again for looking into it!

Supacoco commented 6 years ago

Hey,

You can use this repository to reproduce the exact issue.

And there is the build result: https://travis-ci.org/Supacoco/wct-tests

Thanks again.

Supacoco commented 6 years ago

I did a little bit of digging. I found out what was causing the TypeError: Cannot read property 'start' of undefined.

It's due to the fact that the tested component is wrapped into another component in the test.

In the test repository I created, there is no need for the component to be wrapped. But in our legacy app we have to do that to mock dependencies passed via attributes.

Bubbit commented 6 years ago

Hey,

Sorry again that it took so long, but I found the issue.

The html files that are in the test folder are also loaded into the coverage plugin and it doesn't handle the wrapper functionality nicely and crashes.

Luckily there is a solution by excluding the test files.

exclude: [ '/test/ ]

inside your wct.conf.js.istanbub

I'm going to add a fix to the new release that will do this by default, but if you add exclude in the config it will take that config and ignore the default.

Supacoco commented 6 years ago

Works like a charm! Thanks a lot for looking into it!

Keep up the good work!