Support Addon Coverage #31

rwjblue · 2016-08-24T22:07:34Z

/cc @EWhite613 @sglanzer

Fixes #14.

…jectApp

Processed addon js files

rwjblue · 2016-08-24T22:08:49Z

I'll squash the commits down before merging (likely into one each for @EWhite613 and myself).

@EWhite613 / @sglanzer: Can y'all test this against some of your addons and confirm things are working properly?

kategengler · 2016-08-24T22:32:53Z

index.js

    );

+    console.log(!fileExists, name, relativePath);


Did you mean to leave this in?

Nope, good catch!

I think this is still in here; I'm seeing a bunch of console logging right now trying to run things on ember-ajax that match this pattern

kategengler · 2016-08-24T22:37:58Z

I haven't actually tried it yet, but I'm excited to do so. Thanks for doing this!

kashiif · 2016-08-24T22:39:59Z

This is exicitng news :). I'll try to test a couple of our addons. Thanks

sandersky · 2016-08-24T22:53:35Z

lib/coverage-instrumenter.js

+  try {
+    return instrumenter.instrumentSync(content, relativePath);
+  } catch (e) {
+    console.error('Unable to cover:', relativePath, '. Try setting useBabelInstrumenter to true. \n', e.stack);


Nice addition!

sandersky · 2016-08-24T22:55:53Z

@rwjblue this PR loos great! Thanks a ton for doing this.

sglanzer-deprecated · 2016-08-25T00:43:42Z

@rwjblue We've run it against a fresh addon and one of our engines, we can test against a few other addons tomorrow as well - looking 👍

rwjblue · 2016-08-25T00:48:17Z

@sglanzer - Awesome, thanks for testing!

Definitely want as many testers (of various different ember-cli versions if possible) as possible here...

toranb · 2016-08-25T02:39:36Z

I can't wait to use this :) great work everyone! Will this be something that easily drops into ember observer I assume?

rwjblue · 2016-08-25T02:45:26Z

Yep, that's the idea...

arjansingh · 2016-08-25T06:58:35Z

👍 Tried it on an addon I have and it "Just Worked (tm)". And more importantly the coverage metrics look accurate.

RobbieTheWagner · 2016-08-25T12:54:08Z

Seems to work for me! Should I add COVERAGE=true in front of the ember try:one in travis.yml?

sglanzer-deprecated · 2016-08-26T02:04:04Z

Checking another addon - is anyone else seeing app and tests directories being included in the results? I tried adding a config/coverage.js with

module.exports = {
  excludes: [
    '*/app/**/*',
    '*/tests/**/*'
  ]
}

but that doesn't appear to have an impact either. Not a huge deal, but would be nice to get those files out.

RobbieTheWagner · 2016-08-30T18:18:02Z

@sglanzer I am seeing the same issue. Not a huge deal for me, but would be nice to not do coverage on tests and app, only the addon folder.

rwjblue · 2016-08-30T18:19:57Z

Hmm. The app/ of the addon should be considered as part of its coverage IMO (it is code provided by the addon after all). Including tests/ seems odd though, we need to dig into that to see what is happening.

sandersky · 2016-08-30T18:24:33Z

I agree with @rwjblue. Addons can provide code in app/ that gets merged into the consumers tree, we still want to ensure that code is covered.

RobbieTheWagner · 2016-08-30T18:25:11Z

@sandersky @rwjblue that is true about the app folder, but I think the issue is most of the app files end up just being export { default } from 'foo/components/bar/component';. No problem from my end leaving app in, but tests should definitely be removed from coverage.

sglanzer-deprecated · 2016-08-30T19:13:48Z

Yeah, I'm on board with keeping app

rwjblue · 2016-08-30T19:21:51Z

@sglanzer - Do you think we should just auto-filter tests/ so we can ship this, then circle around and see if we can avoid instrumenting more than we need to during the build?

sglanzer-deprecated · 2016-08-30T19:25:37Z

I'm cool with that - I'd prefer to start using a release instead of this fork 👍

rwjblue · 2016-08-30T19:26:04Z

Hehe, confirm.

lukesargeant · 2016-08-31T21:30:45Z

ember-cli: 2.7.0
ember-cli-mocha: 0.10.4
No additional coverage config

Running 'ember test' passes 23 tests
Running 'COVERAGE=true ember test' passes the 23 tests
NPM linking to a local copy of this PR branch then running 'COVERAGE=true ember test' fails a 24th test:

not ok 24 PhantomJS 1.9 - Global error: INVALID_STATE_ERR: DOM Exception 11: An attempt was made to use an object that is not, or is no longer, usable. at http://localhost:7357/4061/tests/index.html?hidepassed, line 48
    ---
        Log: |
            { type: 'error',
              text: 'INVALID_STATE_ERR: DOM Exception 11: An attempt was made to use an object that is not, or is no longer, usable. at http://localhost:7357/4061/tests/index.html?hidepassed, line 48\n' }

sglanzer-deprecated · 2016-09-01T00:35:11Z

@lukesargeant Any additional info? Do you get similar results on a newer browser (e.g. Phantom 2.x, Chrome, FireFox)

lukesargeant · 2016-09-01T08:00:22Z

The failure is in Phantom 1.9.8
Chrome: all 23 passing.
I'll hopefully have more time to look into this later, but for now I've uploaded my WIP addon:

https://github.com/lukesargeant/ember-box-utils

The only method that touches the dom is:

static fromElement(el) {
    if (el === window) {
      return new Box({ top: 0, left: 0, bottom: window.innerHeight, right: window.innerWidth });
    } else {
      const rect = el.getBoundingClientRect();
      return new Box({ top: rect.top, left: rect.left, bottom: rect.bottom, right: rect.right });
    }
  }

I imagine the problem is here, or in some tear down in the tests themselves.
I haven't reviewed the diff of this PR but i'm surprised it would cause/reveal a global error.

sglanzer-deprecated · 2016-09-01T13:54:40Z

Yeah, I'd be surprised by that as well - my only thought is that the instrumentation wrapper (basically a function call that wraps each line to let us know if it was executed) might be exposing an error that was occurring, but hidden otherwise? Slightly grasping at straws for an explanation of this one without doing some real debugging.

It's good to see that Chrome behaves correctly - Phantom 1.x runs an old version of webkit and has given me problems in the past, we avoid it for most of our testing.

lukesargeant · 2016-09-01T15:35:03Z

Good news. Reinstalled everything, re-tested and the issue went away.

[EDIT] The issue was actually experienced on master, NOT on this branch. Apologies. This commit seems to have introduced the issue:

f2ba0e8

[EDIT] Raised a separate issue #32

sglanzer-deprecated · 2016-09-01T16:22:57Z

Sexy - that's good news - thanks a ton for the investigation @lukesargeant

sglanzer-deprecated · 2016-09-07T15:58:06Z

Any further blockers on merging this? Auto-filtering tests?

RobbieTheWagner · 2016-09-07T16:50:19Z

+1 would love to switch all my apps and addons to this!

arjansingh · 2016-09-07T17:18:47Z

👍 ditto

On Wednesday, September 7, 2016, Robert Wagner notifications@github.com
wrote:

+1 would love to switch all my apps and addons to this!

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#31 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABhksW1PTx_EQzzZcShO_iNVR_70utyBks5qnutMgaJpZM4JsgKb
.

Arjan

sandersky · 2016-09-07T17:43:01Z

The only issue I see with all of this work which shouldn't block a merge is the path for files coming from the addon directory are in the coverage as modules/<name-of-addon>/path/to/file instead of addon/path/to/file. This will prevent editor plugins that highlight code coverage and services like Coveralls from properly reporting coverage on these files.

RobbieTheWagner · 2016-09-07T17:58:53Z

@sandersky doesn't something similar happen with blanket? I know I have to rewrite the names to get accurate reports.

sglanzer-deprecated · 2016-09-07T18:03:11Z

Yeah, I didn't dig enough to figure out why the path was modules, we would need to go back further in the build lifecycle to pick up the "real" path - it's a legit point, but as @sandersky says, I think it can be addressed as a separate issue

lukesargeant · 2016-09-09T13:03:59Z

@sandersky +1 for re-writting module/ to addon/ to facilitate editor plugins. Shouldn't block though since this is still useful, but will be the first new issue to raise :-)

RobbieTheWagner · 2016-09-14T14:15:42Z

So do we want to try to fix the paths now or are there any issues blocking the merging of this?

sglanzer-deprecated · 2016-09-14T15:05:57Z

None that I know of, just waiting for the button to be pushed

kategengler · 2016-09-14T15:10:34Z

I'm deferring to @rwjblue on this, just to be explicit about it.

On Wednesday, September 14, 2016, Steven Glanzer notifications@github.com
wrote:

None that I know of, just waiting for the button to be pushed

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#31 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAbHOjxpsZUxRzYHMg3yRTwfWvk45d_Uks5qqA1WgaJpZM4JsgKb
.

rwjblue · 2016-09-14T15:15:52Z

Released as 0.3.0

rwjblue and others added 7 commits June 29, 2016 13:09

[WIP] Spike on addon support.

cfdd6df

Try catch around instrumenter. So it spits out a clearer error

88adbbd

Fix test _doesFileExistInCurrentProject -> _doesFileExistInCurrentPro…

d7fb5d1

…jectApp

Remove console.log

1e26426

use processedAddonJsFiles

5ea334d

Get rid of InstrumentJs put back preprocessTree

53bf3c2

Merge pull request #30 from EWhite613/processedAddonJsFiles

ac2be3f

Processed addon js files

rwjblue mentioned this pull request Aug 24, 2016

Support Addon Coverage #14

Closed

kategengler reviewed Aug 24, 2016
View reviewed changes

sandersky reviewed Aug 24, 2016
View reviewed changes

lukesargeant mentioned this pull request Sep 1, 2016

Mocha phantom addon coverage issue in master causing a global error. #32

Closed

rwjblue merged commit e5d3955 into master Sep 14, 2016

san650 mentioned this pull request Sep 14, 2016

Add code coverage san650/ember-cli-page-object#179

Closed

This was referenced Sep 14, 2016

Add code coverage #34

Closed

Add code coverage ember-cli/ember-ajax#163

Closed

rwjblue deleted the addon-spike branch August 31, 2020 21:22

Support Addon Coverage #31

Support Addon Coverage #31

Conversation

rwjblue commented Aug 24, 2016 • edited Loading

rwjblue commented Aug 24, 2016

kategengler Aug 24, 2016

Choose a reason for hiding this comment

rwjblue Aug 24, 2016

Choose a reason for hiding this comment

alexlafroscia Sep 15, 2016

Choose a reason for hiding this comment

kategengler commented Aug 24, 2016

kashiif commented Aug 24, 2016

sandersky Aug 24, 2016

Choose a reason for hiding this comment

sandersky commented Aug 24, 2016

sglanzer-deprecated commented Aug 25, 2016

rwjblue commented Aug 25, 2016

toranb commented Aug 25, 2016

rwjblue commented Aug 25, 2016

arjansingh commented Aug 25, 2016 • edited Loading

RobbieTheWagner commented Aug 25, 2016

sglanzer-deprecated commented Aug 26, 2016

RobbieTheWagner commented Aug 30, 2016

rwjblue commented Aug 30, 2016

sandersky commented Aug 30, 2016

RobbieTheWagner commented Aug 30, 2016

sglanzer-deprecated commented Aug 30, 2016

rwjblue commented Aug 30, 2016

sglanzer-deprecated commented Aug 30, 2016

rwjblue commented Aug 30, 2016

lukesargeant commented Aug 31, 2016

sglanzer-deprecated commented Sep 1, 2016

lukesargeant commented Sep 1, 2016

sglanzer-deprecated commented Sep 1, 2016 • edited Loading

lukesargeant commented Sep 1, 2016 • edited Loading

sglanzer-deprecated commented Sep 1, 2016

sglanzer-deprecated commented Sep 7, 2016

RobbieTheWagner commented Sep 7, 2016

arjansingh commented Sep 7, 2016

sandersky commented Sep 7, 2016 • edited Loading

RobbieTheWagner commented Sep 7, 2016

sglanzer-deprecated commented Sep 7, 2016

lukesargeant commented Sep 9, 2016

RobbieTheWagner commented Sep 14, 2016

sglanzer-deprecated commented Sep 14, 2016

kategengler commented Sep 14, 2016

rwjblue commented Sep 14, 2016

rwjblue commented Aug 24, 2016 •

edited

Loading

arjansingh commented Aug 25, 2016 •

edited

Loading

sglanzer-deprecated commented Sep 1, 2016 •

edited

Loading

lukesargeant commented Sep 1, 2016 •

edited

Loading

sandersky commented Sep 7, 2016 •

edited

Loading