Integrate ember-native-dom-helpers #325

ro0gr · 2017-08-20T21:20:01Z

addresses #311
reviewers @san650 @simonihmig

ember-native-dom context allows not to rely on global ember test helpers(click, fillIn,..).
We still use jquery due to custom selectors capabilities(:is, :contains etc)

In order to allow ember application not to use jquery while keeping a backward compatibility ember-cli-page-object now bundles its own jquery copy namespaced as '-jquery'.

To enable native dom helpers in your tests, put this lines into your tests/test-helper.js:

// tests/test-helper.js
...
import { useNativeDOMHelpers } from 'ember-cli-page-object/extend';

useNativeDOMHelpers(); // this will replace classic contexts with `e-n-d-h`

Tests

I've expanded property tests with [native-dom] mode for both integration and acceptance contexts. It seems a little not optimal however it gives a solid code coverage with a minimal efforts.

Todo

ro0gr · 2017-08-20T21:51:46Z

addon/-private/execution_context/acceptance.js

+    } else {
+      let testsContainer = this.testContext ?
+        this.testContext._element :
+        '#ember-testing';


This is definitely not the best thing to be hard-coded

ro0gr · 2017-08-20T21:55:07Z

addon/-private/execution_context/acceptance.js

    guardMultiple(result, selector, options.multiple);

    return result;
  },

+  $(selector, container) {


copied from integration context https://github.com/san650/ember-cli-page-object/pull/325/files#diff-401dad0d4f56e66a450e3abf76d4ef5cR66

ro0gr · 2017-08-20T22:51:31Z

Actually fillIn helper is not used currently due to support of [contenteditable]

ro0gr · 2017-08-20T23:14:24Z

Seems like e-n-d-h can absorb most of differences between acceptance and integration contexts.

simonihmig

Hey @ro0gr, great that you have started working on this. I have not so many comments about the implementation details, mainly because I am not that familiar with the code base. But I will write some other thoughts down later...

simonihmig · 2017-08-21T20:58:43Z

addon/-private/execution_context/acceptance.js

-    triggerEvent(selector, container, 'input');
-    triggerEvent(selector, container, 'change');
+    triggerEvent($selection, 'input');
+    triggerEvent($selection, 'change');


Can't we simply use the built-in fillIn helper of e-n-d-h here?

Ah, just saw your other comment, so ignore this...

simonihmig · 2017-08-21T21:03:25Z

index.js

@@ -11,6 +11,9 @@ module.exports = {
          enabled: this._shouldIncludeFiles(),
          import: ['index.js']
        };
+      },
+      jquery: {
+        import: ['dist/jquery.js'],


What happens here when your app has not opted out of using jQuery? Do we then bundle jQuery twice?

This needs to be verified.
Anyways seems like it's not a big deal cause it's for testing only.
Also this way we can guarantee that we don't consume jquery from ember

I've just used this branch on a project with jquery@1.11.3 included. As a result both jquery versions are bundled into vendor.js.

The interesting thing is that 1.11.3 is still used by ember and v3.2.1 by e-c-p-o so their are no in conflict.

upd: tested in addon with ember originated from bower

all the jquery plugins should be included before e-c-p-o. Otherwise they will extend e-c-p-o's jquery instead of Ember.$ :/

simonihmig · 2017-08-21T21:39:18Z

IIUC you have jQuery still around as a compatibility layer for jQuery-specific selectors like :eq(), right? I cannot remember that I have ever used this in tests. So what about the following other approach:

we keep the existing execution contexts around basically unchanged
we add additional ones (maybe even unified for acceptance and integration?) based on e-n-d-h, that do not require jQuery at all. Obviously non-standard selectors will not work with them. Users have a way to opt in into the new native execution contexts, e.g. by setting some option in ember-cli-build.js, so some broccoli funnel replaces the default context files with those new native-based ones.

This way

we could keep the existing ones untouched, reducing the risk to break something
the implementation of the new native contexts could focus on a native-only implementation, making this easier to reason about. And they would not need to be backwards compatible!
these new ones could land with some experimental support, for users to try this out
once we are happy with them, we can declare them stable, and add some deprecation message to the old ones
with a major version bump the old ones could be dropped and the new ones become the new standard

What's your thoughts on this? @ro0gr @san650

cc @cibernox interested in chiming in here?

ro0gr · 2017-08-21T22:19:33Z

@simonihmig thank you for your thoughts.

IIUC you have jQuery still around as a compatibility layer for jQuery-specific selectors like :eq(), right?

Yes. I think all the specific selectors can be found in Selector class

we add additional ones (maybe even unified for acceptance and integration?) based on e-n-d-h, that do not require jQuery at all

Unfortunately I think it's impossible right now. Lots of properties rely on buildSelector() which builds selectors with jq like filters. It means that with the native context getting of nth item from collection will not work.

we could keep the existing ones untouched

~~nah 😋 Currently I'm working on merging contexts. Seems like e-n-d-h absorbs most of the differences between integration/acceptance modes.~~
I've added 2 native execution contexts co-located with the current ones. You can switch contexts with useNativeDOMHelpers(true|false) right now.
This way we don't touch existing execution contexts.

coveralls · 2017-08-22T19:58:15Z

Coverage decreased (-6.9%) to 90.819% when pulling 5a3106b on ro0gr:native-dom-helpers into 88fad42 on san650:master.

ro0gr · 2017-08-22T20:04:04Z

I agree with @simonihmig that leaving classical contexts untouched is a good thing. So I've reverted all the changes in classic contexts with global ember helpers.

In order to enable e-n-d-h now you can use useNativeDOMHelpers:

// tests/test-helper.js
...
import { useNativeDOMHelpers } from 'ember-cli-page-object/extend';

useNativeDOMHelpers(); // this will replace classic contexts with `e-n-d-h`

Currently it doesn't accept any args.

Users have a way to opt in into the new native execution contexts, e.g. by setting some option in ember-cli-build.js, so some broccoli funnel replaces the default context files with those new native-based ones.

I've thought about build time config for this. But I think this way it will be harder to test all the contexts inside a single e-c-p-o test suite. So I've chosen run-time switch in test-helper.js

coveralls · 2017-09-21T15:48:40Z

Coverage decreased (-6.9%) to 90.819% when pulling 1de4377 on ro0gr:native-dom-helpers into 88fad42 on san650:master.

coveralls · 2017-09-26T10:46:07Z

Coverage decreased (-6.9%) to 90.819% when pulling 6e722ee on ro0gr:native-dom-helpers into 88fad42 on san650:master.

coveralls · 2017-09-30T13:13:43Z

Coverage decreased (-6.9%) to 90.819% when pulling 7c4982b on ro0gr:native-dom-helpers into 88fad42 on san650:master.

ro0gr · 2017-09-30T13:25:09Z

Let's assume the following:

jquery is excluded from the app
you have installed this branch of ecpo (before "truly isolate jquery" commit)
some sly jquery plugin is left in your code base by accident

As a result ember s and ember t will work because this jquery plugin is able to use jquery from ecpo.
At the same time staging/production builds are getting unusable as long as ecpo is stripped from production builds.

I've ended up in similar situation. truly isolate jquery tries to address this issue by calling .noConflict() and namespacing jquery as a pivate module '-jquery'. This trick is happening right after local jquery version is imported.

- use own jquery

ro0gr · 2017-10-04T21:49:03Z

tests/helpers/properties.js

-    beforeEach() {
-      this.adapter = new AcceptanceAdapter(AcceptanceExecutionContext);
-    },
+  [true, false].forEach(_useNativeDOMHelpers => {


I'm not a big fan of this. But it gives us an ability to test native-dom-helpers almost for free. So we can use existing test suite as a baseline.
I have few ideas on how to improve testing story, but I prefer to leave it for a separate issue/pr.

ro0gr · 2017-10-04T21:50:50Z

index.js

@@ -23,6 +30,11 @@ module.exports = {

    this.app = app;

+    if (this._shouldIncludeFiles()) {
+      this.import('vendor/shims/-jquery.js', { prepend: true });
+      this.import('vendor/ecpo-jquery/dist/jquery.js', { prepend: true });


ensure shims goes right after our private jquery instance

Should we put a comment here indicating that this ordering is important for shims/-jquery to work? The comment in shims/-jquery is really helpful, and I think the assumption we're making there about ordering will be more secure if there is also a warning here, where that ordering is defined.

ro0gr · 2017-10-04T21:53:09Z

vendor/shims/-jquery.js

@@ -0,0 +1,11 @@
+(function() {
+  var jq = self['$'].noConflict();


here we heavily rely on the assumption that we included jquery right a before this script
https://github.com/san650/ember-cli-page-object/pull/325/files#diff-168726dbe96b3ce427e7fedce31bb0bcR35

this way noConflict gives us our jquery instance and disposes it from the window.

I think I'll inline some comments in code a little bit later

ro0gr · 2017-10-04T21:55:15Z

addon/-private/execution_context/integration.js

@@ -33,7 +33,9 @@ IntegrationExecutionContext.prototype = {
  },

  // Do nothing in integration test
-  visit: $.noop,
+  visit() {
+    throw new Error('visit is not supported in integration mode');


should I rollback this?

I like this. If someone calls visit in an integration test, they'll get some feedback that will help them learn more about how to use ember-cli-page-object. I wonder...what happens if you set the context in an integration test and then forget to remove it? Is it possible that we would end up seeing this message in an acceptance test due to improper cleanup of an integration test? If so, then the error message should probably address this. Suggest using throwBetterError for consistency with error messaging elsewhere.

I've tried to use throwBetterError here. Unfortunately it doesn't work out of the box because throwBetterError is currently coupled exlusively to element find failures.

This pr is already quite large. I've reverted original silent behavior.

For future reference, here's an example of where throwBetterError is used outside the context of element find failures: https://github.com/san650/ember-cli-page-object/blob/master/addon/-private/properties/alias.js#L86.

ro0gr · 2017-10-05T05:52:17Z

Currently all the trigger tests which test keypress 13 are failing on CI in native-dom-helpers mode.
I see similar issue with IE currently. At the same time it works perfectly with chrome and firefox.

I'm sure the issue originated from fireEvent.
The next step is to fix this.

update: cibernox/ember-native-dom-helpers#112

magistrula · 2017-10-05T03:50:14Z

addon/-private/execution_context/integration.js

@@ -33,7 +33,9 @@ IntegrationExecutionContext.prototype = {
  },

  // Do nothing in integration test
-  visit: $.noop,
+  visit() {
+    throw new Error('visit is not supported in integration mode');


I like this. If someone calls visit in an integration test, they'll get some feedback that will help them learn more about how to use ember-cli-page-object. I wonder...what happens if you set the context in an integration test and then forget to remove it? Is it possible that we would end up seeing this message in an acceptance test due to improper cleanup of an integration test? If so, then the error message should probably address this. Suggest using throwBetterError for consistency with error messaging elsewhere.

magistrula · 2017-10-05T03:53:26Z

addon/-private/execution_context/context.js

@@ -0,0 +1,131 @@
+import $ from '-jquery';


Would it be more appropriate to call this file native-context.js?

yes I also thought about it.
Actually I like dom-context more. native sounds misleading to me a little bit. Maybe native-dom-context is more appropriate here?

native-dom-context sounds good to me!

magistrula · 2017-10-05T03:56:01Z

addon/-private/execution_context/context.js

+    throw new Error('not implemented');
+  },
+
+  // Do nothing in integration test


I think this comment might be left over from an earlier copy/paste?

magistrula · 2017-10-05T04:07:59Z

addon/-private/execution_context/context.js

+      return $(selector, container);
+    } else {
+      let testsContainer = this.testContext ?
+        this.testContext._element :


Would be nice not to have to use the private _element property, and it looks like there's support in the community for making element part of the public API. Suggest adding a TODO comment with a link to this issue so that we can update this once that issue is handled.

#ember-testing one line above also doesn't look very nice.

magistrula · 2017-10-05T04:16:40Z

addon/-private/execution_context/context.js

+  },
+
+  fillIn(selector, container, options, content) {
+    let el = this.$(selector, container)[0];


Since we are selecting only the first matched element here, what would happen if someone called fillIn in a situation where { multiple: true } is present? What do we gain from passing an element into fillElement instead of a jQuery selection?

I've never used fillIn with { multiple: true } :)

I'll update it. thanks.

According to fillable API docs there is no support for multiple option. Does fillable intend to support this option?

You're right that it's not called out in the docs, but fillable does work with { multiple: true }, so it's probably safest to preserve that functionality at this point.

OK, will do. Thx

magistrula · 2017-10-05T04:22:04Z

addon/extend.js

+import IntegrationNativeContext from './-private/execution_context/integration-native';
+import AcceptanceNativeContext from './-private/execution_context/acceptance-native';
+import IntegrationClassicContext from './-private/execution_context/integration';
+import AcceptanceClassicContext from './-private/execution_context/acceptance';


I like the idea of contrasting naming like Native vs Classic. Suggest IntegrationJQueryContext and AcceptanceJQueryContext to increase clarity. Might even be good to rename execution_context/integration and execution_context/acceptance to execution-context/integration-jquery and execution-context/acceptance-jquery.

Suggest IntegrationJQueryContext and AcceptanceJQueryContext to increase clarity.

Currently all the 4 actual contexts use jquery to find elements.~~I'd suggest to rename all the contexts.~~ I like your idea to rename existing contexts.

How about?

acceptance-ember-context,

integration-ember-context

acceptance-native-dom-context

integration-native-dom-context

It seem too verbose to me. But I'm ok with it.

How about acceptance-jquery-event-context, acceptance-native-event-context, etc? And then, native-event-context instead for context.js above? If the file names are too long, we could add acceptance and integration folders, each of which could include native-event-context and jquery-event-context.

How about acceptance-jquery-event-context

i'm not sure why do you refer to event, triggering of events is only a part of context

jquery is not dedicated only for existing contexts. All of contexts(including native-dom) use jquery to find elements to find DOM elements. More details here

native-event-context instead for context.js above?

yes, sure. Hovewer I think we agreed on native-dom-context name

It seems that the core difference between these contexts is the way in which the events are triggered, so that's why I'm suggesting filenames with event in them as well as native-event-context instead of native-dom-context. Not a big deal either way...just pondering how to achieve the most clarity.

Now I see your point about events. However acceptance-jquery-event-context is still not explanatory enough cause it also uses global ember test helpers(find, click).
I also agree with you that it's not a big deal cause it's not a part of public API.
I think it might be better not too touch exiting context names at all until a better name eventually comes to us :)

ro0gr · 2017-10-15T12:47:09Z

hm.., I've just checked IE and all the tests started to pass(even in IE9 emulation mode) after keyEvent fix. However CI with phantomjs still fails.

I'm not sure if we want to fix issues in phantomjs because it's abandoned in community now...

ro0gr · 2017-10-18T18:48:07Z

I think the best way to avoid issue with phantomJS is to replace it with another browser #333

magistrula · 2017-10-28T16:29:20Z

addon/-private/execution_context/native-dom-context.js

+    let elements = this.$(selector, container).toArray();
+
+    elements.forEach((el) => {
+      fillElement(el, content, {


Todo:

Update function signature of fillElement to indicate that the first param is an element

Update the usage of fillElement in the other places where it is used so that we are always passing an element and not a jQuery selection.

magistrula · 2017-10-28T16:56:02Z

index.js

@@ -23,6 +30,11 @@ module.exports = {

    this.app = app;

+    if (this._shouldIncludeFiles()) {
+      this.import('vendor/shims/-jquery.js', { prepend: true });
+      this.import('vendor/ecpo-jquery/dist/jquery.js', { prepend: true });


Should we put a comment here indicating that this ordering is important for shims/-jquery to work? The comment in shims/-jquery is really helpful, and I think the assumption we're making there about ordering will be more secure if there is also a warning here, where that ordering is defined.

pzuraq · 2017-10-31T19:35:53Z

I tried this branch locally to see if it would work (we use native dom events so this is going to be necessary for us) and I'm getting define is not a function, appears that the jquery shim file is getting prepended before define is actually assigned to be function. This is with ember-cli@2.14.0

ro0gr · 2017-10-31T20:05:02Z

@pzuraq thank you for report. I've tested it against ember-cli@2.12 and 2.16 and haven't noticed such issue yet.

I'll test it with 2.12. However some more additional details would be helpful:

what's you npm version
is it happening inside an app or addon
do you do npm link or just specify a git URL in your package.json

Thanks

pzuraq · 2017-10-31T20:07:07Z

We're using yarn@v1.1.0
This is happening inside an addon
I'm using a git URL in package.json

pzuraq · 2017-10-31T20:21:11Z

addon/-private/execution_context/acceptance-native-dom.js

+};
+
+AcceptanceExecutionContext.prototype.runAsync = function(cb) {
+  window.wait().then(() => {


This appears to be the last dependency on a global test helper, which means we still have to use setContext to be able to use PageObjects in integration tests. We can do the same thing ember-native-dom-helpers does here to make this completely independent:

import wait from 'ember-test-helpers/wait'; (window.wait || wait)().then(() => {});

You are right , seems like it should be fixed.

ro0gr · 2017-11-05T14:45:36Z

@pzuraq I've updated handling of jquery per you suggestion in slack.
Now we store it in a temporary global variable in the very beginning and later expose it as an amd module.

ro0gr · 2017-11-11T21:16:05Z

superseded by #339

ro0gr commented Aug 20, 2017

View reviewed changes

simonihmig reviewed Aug 21, 2017

View reviewed changes

ro0gr mentioned this pull request Aug 29, 2017

change property unit tests to assert against real DOM #327

Merged

ro0gr force-pushed the native-dom-helpers branch from 5cf776c to 7fdb8aa Compare October 3, 2017 21:05

ro0gr and others added 13 commits October 4, 2017 22:27

move to dom-native-helpers

afab45e

- use own jquery

unify elements search

715c2dd

unify contexts

9455cce

add useNativeDOMHelpers

5e8b1f2

trigger first selected element

6a22b55

conditional jquery import

ab28fab

prepend jquery

f3b8748

truly isolate jquery

b0fabcb

bump ember-cli-node-assets

6e3b06d

work-around deprecated keyCode property of KeyboardEvent

2f35680

add native-context to acceptance tests

144c4fe

update tests to work with native-dom-helpers

16fd45b

don't destruct jquery collection

50d8d33

ro0gr force-pushed the native-dom-helpers branch from 7fdb8aa to 50d8d33 Compare October 4, 2017 20:40

isolate jquery truly truly

01d28c6

ro0gr commented Oct 4, 2017

View reviewed changes

magistrula reviewed Oct 8, 2017

View reviewed changes

ro0gr added 3 commits October 15, 2017 14:55

rename contexts from native to native-dom

a56074e

Delete global jQuery reference

f158437

properly handle ember-native-dom keyEvent

56efa22

do nothing on visit in integration contexts

1eaafc7

ro0gr changed the title ~~[wip] replace ember global test helpers with native dom helpers~~ Integrate ember-native-dom-helpers Oct 15, 2017

ro0gr mentioned this pull request Oct 16, 2017

[Feature Request] Remove jQuery dependency #289

Open

add support for multiple fillIn

11dded4

magistrula reviewed Oct 28, 2017

View reviewed changes

pzuraq reviewed Oct 31, 2017

View reviewed changes

split jquery shim into global and amd steps

253f2d1

ro0gr mentioned this pull request Nov 11, 2017

Add native-events mode #339

Merged

1 task

ro0gr closed this Nov 11, 2017

		@@ -0,0 +1,11 @@
		(function() {
		var jq = self['$'].noConflict();

Integrate ember-native-dom-helpers #325

Integrate ember-native-dom-helpers #325

Conversation

ro0gr commented Aug 20, 2017 • edited Loading

Tests

Todo

Choose a reason for hiding this comment

ro0gr Aug 20, 2017 • edited Loading

Choose a reason for hiding this comment

ro0gr commented Aug 20, 2017

ro0gr commented Aug 20, 2017

simonihmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Aug 21, 2017

ro0gr commented Aug 21, 2017 • edited Loading

coveralls commented Aug 22, 2017

ro0gr commented Aug 22, 2017 • edited Loading

coveralls commented Sep 21, 2017

coveralls commented Sep 26, 2017

coveralls commented Sep 30, 2017

ro0gr commented Sep 30, 2017 • edited Loading

ro0gr Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr commented Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

ro0gr commented Oct 15, 2017 • edited Loading

ro0gr commented Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pzuraq commented Oct 31, 2017

ro0gr commented Oct 31, 2017

pzuraq commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr commented Nov 5, 2017

ro0gr commented Nov 11, 2017

ro0gr commented Aug 20, 2017 •

edited

Loading

ro0gr Aug 20, 2017 •

edited

Loading

ro0gr Sep 21, 2017 •

edited

Loading

ro0gr commented Aug 21, 2017 •

edited

Loading

ro0gr commented Aug 22, 2017 •

edited

Loading

ro0gr commented Sep 30, 2017 •

edited

Loading

ro0gr Oct 4, 2017 •

edited

Loading

ro0gr Oct 4, 2017 •

edited

Loading

ro0gr Oct 15, 2017 •

edited

Loading

ro0gr commented Oct 5, 2017 •

edited

Loading

ro0gr Oct 8, 2017 •

edited

Loading

ro0gr Oct 8, 2017 •

edited

Loading

ro0gr Oct 8, 2017 •

edited

Loading

ro0gr Oct 12, 2017 •

edited

Loading

ro0gr Oct 12, 2017 •

edited

Loading

ro0gr commented Oct 15, 2017 •

edited

Loading

ro0gr commented Oct 18, 2017 •

edited

Loading