Move all assets to test-support.js instead of vendor.js #399

simonihmig · 2018-04-07T07:30:13Z

This fixes #397 and also implements the other changes suggested there: All (own and imported) JS is moved from vendor.js to test-support.js. The repo's code is now in the addon-test-support tree, and the trickery in treeForAddonTestSupport() (shamelessly stolen from ember-native-dom-helpers) makes sure the imports have not changed (i.e. no test-support/ path added to imports).

I tried this fork with a large app, and was working fine. Didn't have to update anything (except package.json), and all still working. I hope I am not missing something though, so please double check if these changes make sense for you! :)

coveralls · 2018-04-07T07:49:41Z

Coverage decreased (-3.2%) to 95.522% when pulling b84e504 on simonihmig:fix-fastboot-jquery into e53564e on san650:master.

simonihmig · 2018-04-07T08:18:35Z

index.js

    // TODO: In order to make the addon work in EmberTwiddle, we cannot use // the `tests` prop til
    // https://github.com/joostdevries/twiddle-backend/pull/28 is merged.
    // return !!this.app.tests;

-    if(process.env && process.env.EMBER_CLI_FASTBOOT) {


Btw, this was used in pre-stable versions of FastBoot, but isn't a thing anymore for a while, so removed it.

simonihmig · 2018-04-07T08:20:53Z

node-tests/unit/addon/addon-tree-test.js

-
-  describe('#treeFor app', function() {
-    treeForTests('app');
-  });


Removed these tests (which were failing before), because I think they don't really apply anymore. app and addon are always empty now, and for addon-test-support there's no need for conditional logic anymore as it is only used in tests anyway.

san650 · 2018-04-07T13:26:02Z

Thanks @simonihmig for working on this!

As you said we have to test this change to validate there are not regressions.

I'll test it on some small projects I have but it would be great if @magistrula and others that have big test suites can test it as well.

ro0gr

I like it. This change allows ember-cli-page-object to be more similar to the rest of existing testing addons.

I believe we still have to change API paths for the docs generator as well:

ember-cli-page-object/docs.js

Line 265 in e53564e

globs: ['addon/-private/{create.js,properties/*.js}' ]

ro0gr · 2018-04-07T14:57:15Z

index.js

+    });
+  },
+
+  _shouldIncludeFiles() {


I think _shouldIncludeFiles was previously needed to exclude ember-cli-page-object assets from the prod builds. Now it should be handled by ember-cli since test-support stuff is not included to the prod builds. So I'd say we can remove it.

Yeah, good catch. Makes sense!

ro0gr · 2018-04-07T15:09:06Z

@simonihmig I've just merged docs updates which produced conflicts in your PR 😫 Sorry for not merging it earlier. Can you please resolve conflicts?

ro0gr · 2018-04-07T16:08:53Z

Hm.. Seems like addon-test-support/ is not included in the code coverage reports.

simonihmig · 2018-04-07T19:00:01Z

Thanks for the quick feedback!

I rebased this, and cleaned this up a bit further.
Removed _shouldIncludeFiles, and changed the node-assets config a bit, as it does not need to be dynamic anymore. This revealed that ceibo was still imported to vendor.js, which should be fixed now as well.

I believe we still have to change API paths for the docs generator as well

I updated this line, but as I am not aware how that works, please double check...

I'll test it on some small projects I have but it would be great if @magistrula and others that have big test suites can test it as well.

Absolutely! I reran the latest changes on one of our apps, and its 1700+ tests (all included) are still all passing.

Hm.. Seems like addon-test-support/ is not included in the code coverage reports.

Sorry, I have no idea how that works! 😝 Also wondered how coverage could have changed significantly. Anything I can do on my side to fix this?

ro0gr · 2018-04-07T19:30:12Z

Also wondered how coverage could have changed significantly. Anything I can do on my side to fix this?

Looks like it should be fixed in the latest of ember-cli-code-coverage. I think we just have to upgrade it.

ro0gr · 2018-04-08T18:03:44Z

I think we just have to upgrade it.

I've just tried ember-cli-code-coverage@1.0.0-beta.3 locally. Unfortunatelly it reports zero coverage for each of file in the addon-test-support/ folder. Probably it's related to the import path tricks borrowed from ember-native-dom-helpers.

ro0gr · 2018-04-17T22:46:51Z

@simonihmig FYI I'm currently working on the code coverage issue on top of your branch. The issue is caused by the fact that ember-cli-code-coverage expects generated code to have a fixed module path (addon-name/test-support). So the possible solution is to change only entries module names(index, extend, macros) and leave the rest of the files under ember-cli-page-object/test-support namespaced.

wip branch: https://github.com/ro0gr/ember-cli-page-object/commits/addon-test-support-coverage

simonihmig · 2018-04-18T19:15:31Z

@ro0gr Hm, I see. Thanks for working on this, and for giving feedback! Hope it works out somehow!

ro0gr · 2018-05-14T22:59:52Z

@simonihmig just merged #400. Thanks for the great idea!

simonihmig · 2018-05-15T10:21:42Z

@ro0gr Sure, thanks for picking this up and getting it across the finish line! :)

simonihmig commented Apr 7, 2018

View reviewed changes

ro0gr requested changes Apr 7, 2018

View reviewed changes

simonihmig added 2 commits April 7, 2018 19:35

Move all assets to test-support.js instead vendor.js

2a30c05

Remove addon-tree-test.js

ece675e

simonihmig force-pushed the fix-fastboot-jquery branch from 81b9a80 to 801d0f9 Compare April 7, 2018 17:54

Cleanup index.js, fix docs.js

b84e504

simonihmig force-pushed the fix-fastboot-jquery branch from 801d0f9 to b84e504 Compare April 7, 2018 18:37

ro0gr approved these changes Apr 7, 2018

View reviewed changes

ro0gr mentioned this pull request Apr 19, 2018

move to addon-test-support + working coverage reports #400

Merged

dfreeman mentioned this pull request May 8, 2018

Improve FastBoot compatibility #401

Closed

ro0gr closed this May 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all assets to test-support.js instead of vendor.js #399

Move all assets to test-support.js instead of vendor.js #399

simonihmig commented Apr 7, 2018

coveralls commented Apr 7, 2018 •

edited

Loading

simonihmig Apr 7, 2018

simonihmig Apr 7, 2018

san650 commented Apr 7, 2018

ro0gr left a comment •

edited

Loading

ro0gr Apr 7, 2018

simonihmig Apr 7, 2018

ro0gr commented Apr 7, 2018 •

edited

Loading

ro0gr commented Apr 7, 2018

simonihmig commented Apr 7, 2018

ro0gr commented Apr 7, 2018

ro0gr commented Apr 8, 2018

ro0gr commented Apr 17, 2018

simonihmig commented Apr 18, 2018

ro0gr commented May 14, 2018

simonihmig commented May 15, 2018

Move all assets to test-support.js instead of vendor.js #399

Move all assets to test-support.js instead of vendor.js #399

Conversation

simonihmig commented Apr 7, 2018

coveralls commented Apr 7, 2018 • edited Loading

simonihmig Apr 7, 2018

Choose a reason for hiding this comment

simonihmig Apr 7, 2018

Choose a reason for hiding this comment

san650 commented Apr 7, 2018

ro0gr left a comment • edited Loading

Choose a reason for hiding this comment

ro0gr Apr 7, 2018

Choose a reason for hiding this comment

simonihmig Apr 7, 2018

Choose a reason for hiding this comment

ro0gr commented Apr 7, 2018 • edited Loading

ro0gr commented Apr 7, 2018

simonihmig commented Apr 7, 2018

ro0gr commented Apr 7, 2018

ro0gr commented Apr 8, 2018

ro0gr commented Apr 17, 2018

simonihmig commented Apr 18, 2018

ro0gr commented May 14, 2018

simonihmig commented May 15, 2018

coveralls commented Apr 7, 2018 •

edited

Loading

ro0gr left a comment •

edited

Loading

ro0gr commented Apr 7, 2018 •

edited

Loading