Support RFC268 tests #357

bendemboski · 2017-12-29T16:57:32Z

I think this PR is best read commit by commit rather than as a single changeset.

This implements an execution context for RFC268-style tests, meaning tests that use setupApplicationTest/setupRenderingTest instead of moduleForAcceptance/moduleForComponent. It's not required, but this execution context will work much better ergonomics-wise with async/await than with promise chaining.

The rfc268 execution context uses the @ember/test-helpers helpers (click, visit, fillIn, etc.), which use native events.

The rfc268 execution context supports call chaining with the same async behavior as existing tests, i.e. waiting in between chained async calls, but when used without chaining, reflects the RFC268 async behavior, i.e. nothing is async unless explicitly awaited.

Still TODO:

Update documentation
Fix test coverage
Run codemode for Javascript modules API

Fixes #355

coveralls · 2017-12-29T17:08:25Z

Coverage decreased (-0.1%) to 97.95% when pulling 321b390 on bendemboski:rfc268 into 5718348 on san650:master.

ro0gr · 2017-12-29T19:46:01Z

I've just ran code-corps-ember test suite(as an example of pretty extensive test suite using ember-cli-page-object) against this branch. Tests are green.

Great job!

ro0gr · 2017-12-29T22:18:41Z

tests/acceptance/rfc268-actions-test.js

+    await page.keys.clickOn('3');
+    page.keys.asyncEqual();
+
+    await waitUntil(() => page.isLoading);


I like this. it makes me think about something like waitFor dsl property:

await page.waitFor();

Yeah, I thought about that too. But then I realized that probably sometimes you want to wait for an element to exist, and sometimes wait for it to be visible. (@ember/test-helpers's waitFor just waits for it to exist). So then I thought maybe an optional callback to verify conditions other than just pure existence...then I thought that doesn't seem to really add any value beyond using waitUntil() directly:

await waitUntil(() => page.foo);

or

await waitUntil(() => page.isFooVisible);

So it's fully doable, but I couldn't convince myself it would add much value...

I think the most common use case is to wait for an element visibility.
So waitFor property could always wait for isVisible of the node. If the user wants to customize a wait condition he can always use a new good waitUntil helper.

However it's ~~totally unrelated~~ out of scope of this PR. I've just realized that it can be a good addition to API.

ro0gr · 2017-12-30T00:43:50Z

package.json

@@ -37,7 +37,7 @@
    "ember-cli-babel": "^6.6.0",
    "ember-cli-node-assets": "^0.2.2",
    "ember-native-dom-helpers": "^0.5.3",
-    "ember-test-helpers": "^0.6.3",
+    "ember-test-helpers": "https://github.com/emberjs/ember-test-helpers.git#v0.7.10",


I think this can be replaced with the latest published @ember/test-helpers@0.7.11

ro0gr · 2017-12-30T01:07:12Z

tests/helpers/properties/application-adapter.js

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


as far as I understand $ here is exactly the same as Ember.$. Probably we should replace all the 'jquery' imports with '-jquery' in tests as well?...

This is something I'm not familiar with -- what's the difference between importing from jquery and -jquery?

We can't rely on Ember.$ cause it can be empty in apps which don't use jquery.
ember-cli-page-object adds its own jquery version in case of app's jquery absence:

https://github.com/san650/ember-cli-page-object/blob/master/index.js#L29-L31

We call this jquery module '-jquery' in order to prevent overlapping with regular 'jquery'.

ember-cli-page-object/vendor/shims/ecpo-jquery.js

Line 22 in 5718348

define('-jquery', [], vendorModule);

In case when Ember.$ is present we also put it into '-jquery' for convenience:

ember-cli-page-object/vendor/shims/project-jquery.js

Line 14 in 5718348

define('-jquery', [], vendorModule);

So I think import from '-jquery' is recommended way to use jquery inside this project.

I'm not sure if we have to change all the jquery imports to -jquery in the tests at least until Ember drops jquery support. But we definitely have to do it in the public code.

ro0gr · 2017-12-30T01:12:54Z

addon/-private/execution_context/rfc268.js

+} from '../better-errors';
+import Ceibo from 'ceibo';
+
+const { $, RSVP: { resolve } } = Ember;


We should not use Ember.$ in the e-c-p-o internally. That would not allow to use this contexts in the app with disabled jquery.

It can be fixed by importing an isolated jquery instance:

ember-cli-page-object/addon/-private/execution_context/native-events-context.js

Line 1 in 5718348

import $ from '-jquery';

Also we can use new modules imports.

import { resolve } from 'rsvp';

Yeah, I was wondering about that -- should I just run the codemod to migrate everything to the new module imports?

Sure, i think that's the quickest way to do it

coveralls · 2017-12-30T01:43:59Z

Coverage increased (+0.1%) to 98.164% when pulling eb639de on bendemboski:rfc268 into 5718348 on san650:master.

coveralls · 2017-12-30T23:32:13Z

Coverage increased (+0.2%) to 98.255% when pulling 1dae87b on bendemboski:rfc268 into 5718348 on san650:master.

bendemboski · 2017-12-31T04:55:53Z

I think this is all ready to go except for updating the documentation. I'm not quite sure how to do that. Options that I can think of:

Keep all the existing examples the same and add a single section somewhere explaining how the RFC268 test API differs (basically just adding awaits everywhere)
Duplicate all the existing examples to also illustrate the new test API syntax
Modify all the existing examples to show the new syntax and then link to a frozen version of the existing documentation site for "legacy" tests

I need some direction here...

ro0gr · 2017-12-31T08:22:23Z

addon/-private/execution_context/rfc268.js

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


we should use -jquery here cause it can be ran in consumers test suites w/o Ember.$ available

snewcomer · 2018-01-01T19:08:53Z

@bendemboski This is huuuuge. Amazing work!

Coming from a point of naivety (and understanding only a tiny fraction of this PR), is it worthwhile to perf test this at all to see if any big regressions (perhaps related to creating the mirror object)? On very large acceptance test suites, memory leaks and such are things I have encountered before.

bendemboski · 2018-01-01T20:44:59Z

@snewcomer here's my understanding of the memory story. A page object's Ceibo tree is a singleton -- it's created by the PageObject.create() call, which happens once per page object when the page object's module is first imported. That's why in your test files you just import your page object, rather than importing a class that you have to instantiate in each test file. So it's kinda immune to memory leaks because it's a singleton and is never destroyed and re-created (it's pre-leaked 😄)! Now we're doubling the amount of memory it consumes by creating the mirror tree, but it's created according to the same pattern and is similarly immune to leaking.

Of course I've made airtight logical arguments about code plenty of times and turned out to be wrong (especially in the areas of perf and memory consumption), so I think doing some perf testing to verify is a fine idea, I just don't personally think the likelihood of a regression is high enough that I feel motivated to make time to do such testing...

coveralls · 2018-01-02T18:26:41Z

Coverage increased (+0.2%) to 98.255% when pulling b01bbf1 on bendemboski:rfc268 into 5718348 on san650:master.

snewcomer · 2018-01-02T18:48:12Z

Agreed. Thanks for the great explanation.

ro0gr · 2018-01-03T11:33:53Z

@bendemboski my 2¢ regarding the docs.

After some reading of docs I can say there are very few examples of moduleFor* usages currently. I can find only moduleForComponent in the integration tests section on the quickstart page.

That makes me think we could add just a tiny section with few RFC268 examples. We could also explain that with this feature we don't need to deal with setupContext(this)/teardownContext() anymore 🍾

basically just adding awaits everywhere

I think await is not very related here. I believe it can be used with current master even w/o RFC268 support.

bendemboski · 2018-01-03T18:16:26Z

@ro0gr await can be used in master without these changes, but with these changes, awaiting (or promise chaining) every helper call is mandatory. Whether in an application (formerly acceptance) or integration (formerly rendering) test, you need to await the helpers because they all wait a tick (which is a 0ms timer) before actually executing, so

page.click('.foo');
assert.ok(fooWasClicked());

will always fail. You need to

await page.click('.foo');
assert.ok(fooWasClicked());

or if you really want to

return page.click('.foo').then(() => {
  assert.ok(fooWasClicked());
});

(but I don't think we should bother documenting the promise chaining syntax -- anybody that doesn't want to use async/await with the new test API is on their own).

ro0gr · 2018-01-03T19:06:41Z

Yes, I totally get the requirement to use await for new contexts. I just mean that existing examples are still valid for tests that use old school moduleFor. I believe moduleFor style remains the default option for v1.x.x of ember-cli-page-object so existing examples can be left untouched. We can just focus on writing a section dedicated to RFC268 for now.

Though I don't feel very strong about that, that's just my understanding.

bendemboski · 2018-01-03T19:15:38Z

Oh yeah, gotcha. Yeah, that makes sense. I agree with treating moduleFor tests as the default/common case and RFC268 as the special case for now. Of course at some point (not exactly sure when) ember-cli is going to switch the default test blueprints to be RFC268 tests, and at some point more users than not be will running RFC268...but I think you're right that for now it makes the most sense to leave all the documentation the same, but add a section that specifically describes the differences when using the RFC268 API.

magistrula · 2018-01-04T18:07:21Z

addon/-private/execution_context.js

+  let context;
+  if (testContext) {
+    context = 'integration';
+  } else if (getTestContext()) {


Could we add some explanatory comments here? Specifically, what do getContext and getTestContext give us, and how do they determine determine whether we should use the integration context or the rfc268 context?

coveralls · 2018-01-04T19:20:40Z

Coverage increased (+0.2%) to 98.728% when pulling 9d565b8 on bendemboski:rfc268 into b15e53c on san650:master.

bendemboski · 2018-01-10T02:11:23Z

@san650 @magistrula I just pushed a commit to fix the version/upgrade issues. The solution was to pull @ember/test-helpers and ember-test-helpers from package.json, and then do runtime detection of whether a new enough @ember/test-helpers is installed when picking execution contexts. I put together a test project that uses ember-try to verify that the difference scenarios that we care about work -- you can check it out here if you like.

I also rebased onto master and updated for focus/blur support.

I think maintaining this PR when the test syntax diverges so much from master is going to quickly get very cumbersome, so if we're not going to figure out the documentation question and get this pushed through pretty soon, I'd like to break off the ember-cli upgrade and test syntax update (basically the first two commits, but with a little cleanup related to ember-test-helpers) into its own PR in such a way that it wouldn't affect users, but would get all the tests on the new syntax, and then get that merged soon.

Does that sound doable?

magistrula · 2018-01-10T06:13:04Z

addon/-private/rfc268-helpers.js

+      return null;
+    }
+  }
+}


I think it would be helpful for this file to have something like this, to simplify the logic/limit the concerns of -private/execution_context.js.

export function hasRfc268TestContext() { // `helpers.getContext()` returns: // - `null`, if @ember/test-helpers is not available // - _____?, if @ember/test-helpers is available but none of the `ember-qunit` // setupTest() methods has been called (e.g., `setupRenderingTest()`) // - qunit test context, if @ember/test-helpers is available and one of the // `ember-qunit` setupTest() methods has been called. let hasValidTestContext = !!helpers.getContext(); if (!hasValidTestContext) { return false; } let hasExpectedTestHelpers = !!helpers.visit; if (!hasExpectedTestHelpers) { throw new Error([ 'You are trying to use ember-cli-page-object with RFC232/RFC268 support', '(setupRenderingContext()/setupApplicationContext()) which requires at', 'least ember-qunit@3.2.0 or ember-mocha@0.13.0-beta.3.' ]); } return true; }

Then, in -private/execution_context.js:

export function getExecutionContext(pageObjectNode) { let pageObjectContext = getContext(pageObjectNode); let context; if (pageObjectContext) { context = 'integration'; } else if (hasRfc268TestContext()) { context = 'rfc268'; } else { context = 'acceptance'; } return new executioncontexts[context](pageObjectNode, pageObjectContext); }

Yeah, I guess I could do that, but there's not a really clean separation of concerns. The logic in hasRfc268TestContext() relies on the fact that we've already ruled out the integration context in order to be sure it's making the right compatibility/upgrade decisions for the user. We have to make page.setContext() force integration mode or we break compatibility with people that have started using ember-cli-page-object with RFC232 tests (which works fine on master).

So the logic is really a cascade of decisions:

Integration test or RFC232 test that is calling page.setContext()? => integration

RFC232 or RFC268 test with visit present? => rfc268

else => acceptance

and it seems to me that the importance of the ordering is harder to express when the decision logic isn't all in the same file.

I don't feel strongly about this, so I'm happy to move the code like you suggest, but my personal judgement is that the loss of locality takes away more from the readability than the (imperfect) separation of concerns adds. But say the word and I'll happily concede the point 😄

Yeah, I say let's go ahead and make these changes. I think these explanatory comments and error handling belong in this file rather than -private/execution_context.js.

Okay, pushed those changes (sorry it took a bit -- was on vacation and not connected for the long weekend)

magistrula · 2018-01-10T06:24:43Z

addon/-private/execution_context.js

+    context = 'integration';
+  } else if (getRfc268Context()) {
+    if (!visit) {
+      throw new Error([


Under what circumstances could we end up here? When would getRfc268Context() return true while visit is undefined?

There were a few minor versions of @ember/test-helpers that had setupRenderingTest() implemented, but did not have the DOM helpers. In one of my projects, I upgraded ember-qunit to one of these versions to start implementing RFC232 rendering tests using ember-cli-page-object, but still using page.setContext() so it was using the integration execution context.

Since it's in my yarn.lock, if I were to update to this version of ember-cli-page-object and delete the page.setContext()s to try to use the RFC268 execution context, I'd get cryptic helper is not a function (or something) errors, so this is meant to clarify what's going on.

There was also a brief window when all the helpers but visit() were implemented, but nobody using ember-cli-page-object could have actually been using them since there was no execution context that called them. So to keep things clean and simple (ha!), we check for visit() which was the last one to be added and guarantees that you can run both RFC232 tests and RFC268 tests.

ro0gr · 2018-01-10T07:26:32Z

addon/-private/helpers.js

@@ -1,8 +1,11 @@
-import Ember from 'ember';
+//import { assign, merge } from '@ember/polyfills';


ro0gr · 2018-01-10T07:26:48Z

addon/-private/helpers.js

@@ -310,4 +313,4 @@ export function getProperty(object, pathToProp) {
  return typeof value === 'function' ? value.bind(propOwner) : value;
 }

-export const assign = Ember.assign || Ember.merge;
+//export const assign = assign || merge;


magistrula · 2018-01-18T04:14:51Z

addon/-private/execution_context.js

+  // So, in order to not break those tests, we need to check for that case
+  // first, and only if that hasn't happened, check to see if we're in an
+  // RFC232/RFC268 test, and if not, fall back on assuming a pre-RFC268
+  // acceptance test, which is the only remaining supported scenario.


This is really clear; awesome.

magistrula · 2018-01-18T04:16:40Z

addon/-private/is-rfc268-test.js

+  // helpers (`click`, `fillIn`, etc.) that the RFC268 execution context uses.
+  // `visit` was the last helper to be added to `@ember/test-helpers`, so we
+  // check for it, and if we can't find it, we can't use the RFC268 execution
+  // context, so we throw an exception.


Again, super helpful in explaining hard-to-discover subtleties/background information.

ro0gr

This is huge but great! I've left some minor comments.

ro0gr · 2018-01-21T21:28:43Z

tests/helpers/properties/rfc268-adapter.js

+    }, expected, message);
+  },
+
+  async wait() {


Seems like this is unused anywhere

Oh yeah, that was left over from a previous iteration -- good catch, thanks!

ro0gr · 2018-01-22T19:21:47Z

tests/unit/-private/properties/blurrable-test.js

-  test('raises an error when the element is not focusable', function(assert) {
-    assert.expect(4);
+  test('raises an error when the element is not focusable', async function(assert) {
+    if (adapter === 'acceptance' || adapter === 'integration') {


I realize assert.expect(4) is a pre-existing thing. But is it helpful in any way?
I think the whole if/else condition can be removed w/o any consequences.

Personally, I wouldn't use assert.expect() in this case since the only way it would fail is if a promise rejected, which would fail the test anyway. But I don't feel comfortable applying that preference to code written so recently, unless @pzuraq agrees that removing it makes sense.

I did it mostly out of habit, with the older style tests it was very easy to have flakiness because assertions would get dropped in race conditions. I don't think there's a reason to keep it at this point.

ro0gr · 2018-01-22T19:43:01Z

tests/unit/-private/properties/blurrable-test.js

-          button: blurrable('button'),
-          contentEditable: blurrable('[contenteditable]'),
-          tabindex: blurrable('[tabindex]'),
+  if (adapter === 'integration' || adapter === 'acceptance') {


can we localize an issue by wrapping only specific asserts for iframe and area? Like you do here https://github.com/san650/ember-cli-page-object/pull/357/files#diff-0a0d420063bf16b383570d0c7cbcad6aR197

No, the underlying test helpers don't raise an error if you try to blur and un-focusable element, so this test doesn't just doesn't make sense to run for the new execution context.

ro0gr · 2018-01-22T20:21:21Z

addon/-private/helpers.js

    return cb($(element));
  });
 }

 /**
- * @private
+ * @public


I think technically it's still a private function cause it's not re-exported to the consumer

A couple of the other helpers that are exported from the file, but not to the consumer, are marked as @public like findClosestValue and objectHasProperty so it seems that the @public and @private are just referring to whether it's exported from the file or not, not whether it's exported to consumers of the addon.

ro0gr · 2018-01-22T20:49:55Z

tests/helpers/properties/rfc268-adapter.js

+    return $(selector, isAlternative ? '#alternate-ember-testing' : '#ember-testing');
+  },
+
+  async throws(assert, block, expected, message) {


not sure if it's the same, but can we use assert.rejects here? At least currently we have the latest qunit installed which provides us with this api.

I don't think it's quite the same...mumble mumble test adapter something something...I can't exactly recall but I think we have to do it just so to keep the test setup working correctly.

alexlafroscia · 2018-02-20T19:38:36Z

I'm not sure where the bug lies, but trying out this PR over the last day, I've had to add some

await wait();

after using clickable where it was not required before.

san650 · 2018-02-20T19:57:22Z

@alexlafroscia was the action defined in a collection? Could it be related to #377?

I don't know if the fix for #377 is included in this branch.

If that's not the case, could you provide an example? So we can triage the error.

bendemboski · 2018-02-20T21:28:08Z

@alexlafroscia are you awaiting the invocation of the clickable? Can you post a code snippet?

alexlafroscia · 2018-02-20T22:54:34Z

It could definitely be related to that issue @san650, it is defined within a collection.

I'm also seeing an issue with click-ing an item within a nested collection (collection item in turn has a collection, where I'm trying to click one of those items)

At this line

https://github.com/san650/ember-cli-page-object/pull/357/files#diff-6940e0eec69729b817c170c2aa6fccc0R76

I have a path like ["wizard", "date_range_groups[0]", "categories[0]"], which expands out to

node['wizard']['date_range_groups[0]']['categories[0]']

when what actually works would be something likle

node['wizard']['date_range_groups'][0]['categories'][0]

The page object looks something like this, in this instance

export default create({
  visit: visitable('/calendars/new/type'),
  wizard: {
    date_range_groups: collection(
      '.date-range-container .date-range-category',
      {
        categories: collection('.dropdown-link', {})
      }
    )
  }
});

and I'm trying to execute

page.wizard.date_range_groups.objectAt(0).categories.objectAt(0).click();

I end up with the error Cannot read property 'categories[0]' of undefined because the code I linked to above it attempting to read node['wizard']['date_range_groups[0]'] which does not exist.

bendemboski · 2018-02-21T21:02:24Z

@alexlafroscia nice catch -- I just pushed a commit resolving the issues with awaiting and chaining calls to actions on collection items.

alexlafroscia · 2018-02-21T21:17:15Z

Awesome! Thanks for that @bendemboski, I pulled the latest from your branch and it's working again 👍

Not only was I able to verify that chaining issue, but I was able to remove a bunch of await wait() invocations after trying to click on items inside a collection.

This updates the private unit test code to use the new async/await syntax for testing. The tests are equivalent to what they used to be, but this syntax will allow running the tests against the new rfc268 test API as well. Other than purely syntactic changes, this also involved a couple of semantic changes: * Replace `visit()` stubbing with actual calls to `visit()` because there's isn't a great way to stub `visit()` in the new test framework * Replace `andThen()`/`wait()` adapter calls with an `await()` call. The `await` call is invoked like this: `await this.adapter.await(someHelper())`. In integration tests, it's a no-op and in acceptance tests it ignores the return value of the helper and just awaits the `wait()` helper (making it equivalent to the previous `andThen()/wait()` invocations). But in the new test framework, it will be implement to `await` the return value of the helper, which is how async waiting in the new test framework works.

This execution context detects and supports tests using the new QUnit modules API. The tests that rely on chaining behavior are currently failing as the question of if/how to support chaining is unresolved.

Perform some trickery (see comment in `create.js`) to allow chained calls to have different async behavior from unchained calls in RFC268 tests.

Move the RFC268 decision logic to its own file, comment that logic plus the overall execution context switching logic more thoroughly, and remove some commented imports that I forgot to remove before.

To support collections properly, we need smarter logic when traversing the chained tree to recognize and correctly handle collection items. Also, since collection items are dynamically generated, they won't have the `_chained` property set on them, so just stop using that entirely as it wasn't really needed.

simonihmig

Awesome work here! I found one issue though...

simonihmig · 2018-02-21T21:54:52Z

addon/-private/compatibility.js

+}
+
+if (require.has('ember-test-helpers/wait')) {
+  helpers.wait = require('ember-test-helpers/wait').default;


This is somehow not working for me. I am getting TypeError: (window.wait || _compatibility.wait) is not a function errors, because helpers.wait is undefined.

I could see that in the debugger. Also when calling require('ember-test-helpers/wait') in the console, I was getting an empty object. After running the (failing) tests, it actually returns the expected result (object with the wait function as the default key)!

I don't really know how requirejs works internally, but maybe the module needs to have been evaluated first, by having it as a module dependency? At least after I added import { default as _wait } from 'ember-test-helpers/wait'; (renamed to prevent a name clash) to the beginning of this file, it was working fine then!

Btw, I am not sure I understand what kind of scenario this is aiming at, when neither @ember/test-helpers nor ember-test-helpers is present? Shouldn't one of them always come as a dependency of the test framework addon?

@simonihmig this is to support legacy apps. ember-test-helpers's wait hasn't always existed. ember-cli-page-object previously had ember-test-helpers as a runtime dependency to be sure wait was present. Now it doesn't, so apps with an old enough ember-qunit/ember-test-helpers won't have the wait helper present and will run into that exception you hit instead of the nice descriptive error message that you didn't get :(

I'm confused about exactly what you're seeing. Are you running ember-cli-page-object's unit tests? Or your own tests using this branch? What version of @ember/test-helpers/ember-test-helpers do you have installed? loader.js isn't that complicated -- if you want to peel back an additional layer you could step into the require call and maybe see why it's giving you back an empty object...

I am running my apps tests, using the master branch of ember-mocha. Should be the latest version of @ember/test-helpers, but will double check.

if you want to peel back an additional layer you could step into the require call and maybe see why it's giving you back an empty object...

Ok, will do that, but not before tomorrow...

Okay. I haven't seen anything like that, and the ember-cli-page-object tests run all the relevant ember-try scenarios, so I'm not sure how to investigate with more info or a sample project that reproduces it. So let me know what you find!

@bendemboski so here is what I got...

First of all I have @ember/test-helpers 0.7.18 installed...

When I step into the require call, I can see that the module is in a pending state, and the exports are empty:

As stated before, when I explicitly import ember-test-helpers/wait in that file, it works, and as you can see the module is in the finalized state and the exports are all there:

This is happening only in a few tests, so I suppose this is depending on the order the modules are required: when wait has been required before by some other module, it is already in the finalized state, and the require gets the correct exports. But when the require call in question here is the first to ever need the module, and it has not declared ember-test-helpers/wait as a dependency (by importing it), it will see a pending module and the exports are still empty.

I'll try to get a repro of this when I have the chance (probably later today or tomorrow), but if you have time, could you try changing this line to

helpers.wait = (...args) => require('ember-test-helpers/wait').default(...args);

and see if it works for you? I'm hoping that moving the require to "runtime" rather than "module load time" (if you know what I mean) will fix it.

Just tried it quickly, and yes, it indeed fixes the problem! 😀

I probably miss something important, but what would be the problem if e-c-p-o would simply add the latest @ember/test-helpers as a dependency? (so you could simply import it and not require all of this)

It used to. @magistrula observed that (maybe due to an ember-cli bug or maybe an intended feature?) if the app also specified a version of @ember/test-helpers, the addon's version of @ember/test-helpers was overriding the version the app specified (see #361 and #366). This seems like risky behavior to accept in order to patch up a very uncommon case (an app having a version of ember-test-helpers in the pretty old and narrow window when it didn't have wait).

Also, when I asked in slack, I think it was @Turbo87 and/or @rwjblue that told me that apps and addons can and should just depend on ember-qunit or ember-mocha to supply @ember/test-helpers, and not list it as a dependency themselves.

Anyway, thanks a bunch for digging in -- I'll make that change to defer the require execution until after modules are loaded.

simonihmig · 2018-02-21T22:01:54Z

addon/-private/is-rfc268-test.js

+    throw new Error([
+      'You are trying to use ember-cli-page-object with RFC232/RFC268 support',
+      '(setupRenderingContext()/setupApplicationContext()) which requires at',
+      'least ember-qunit@3.2.0 or ember-mocha@0.13.0-beta.3.'


FWIW, that version of ember-mocha does not support RFC268-based tests. It might ship with an appropriate version of @ember/test-helpers, but it does not yet provide the setupRenderingTest()/setupApplicationTest() functions to setup such tests, as ember-qunit does...

I know that for sure, because my PR for adding those just got merged today 😉

Heh, okay, should I optimistically change the version to 0.13.2 then?

Hm, not sure what version that would be... @Turbo87 any thoughts?

it's actually worse than that. even with ember-qunit@3.2.0 and visit available you can't be sure that you can actually use it, because you might be dealing with a testsuite that still uses the old testing APIs and those don't work with the new visit() function

@Turbo87 this check occures only when we use the new testing API. I think the question here is which minimal versions of ~~ember-qunit and~~ ember-mocha will provide a working visit() helper?

visit was added in @ember/test-helpers@0.7.9, which was first required by ember-qunit@3.2.0, which is why the error references that version of ember-qunit.

@ro0gr is correct -- the question is when ember-mocha will support it. As far as I understand, @simonihmig's PR to add full support was just merged, so will that be released as 0.13.2?

ro0gr · 2018-02-21T22:33:23Z

addon/-private/execution_context/rfc268.js

+          let [ indexStr, index ] = match;
+          let name = key.slice(0, -indexStr.length);
+          node = node[name].objectAt(parseInt(index, 10));
+        } else if ((match = /\((\d+)\)$/.exec(key))) {


Since legacy collection is deprecated and we are going to add new test helpers support to v2.0 I think we should not care about legacy collections.

Won't that make migrating pretty painful? I still have a bunch of code using legacy collections that I'm gonna migrate sometime, but haven't had time to yet since new collections were released so recently. The new test API has been released for quite some time and especially with Ember 3.0, many users are already using it. Do we really want to block users from using the new test API until they migrate all of their collections? Especially since the code is already written and tested 😄

Do you propose to release this as a part of v1 or to preserve legacy collections support in v2?

I think we can't do the former cause we change the way properties are defined which is a part of public API I believe. On the other hand preserving legacy collections support in v2 seems a bit strange(but acceptable at least to me).
mhm.. Maybe it's a time for collections codemod...

Do you propose to release this as a part of v1 or to preserve legacy collections support in v2?

Migrating to v2 of page object is going to be a lot of work for people like @bendemboski said, I think we should strive for having this PR merged in for v1

I think we can't do the former cause we change the way properties are defined which is a part of public API I believe

Can you expand a bit on this? I'm not sure to understand what would break.

I'm reffering to async props like this https://github.com/san650/ember-cli-page-object/pull/357/files#diff-52672839c703d1e4a3286b560cd21215R78. We should now return an async operations. I suspect this change might break someones custom props

Actually I'm not sure how wide-spread is custom props in the wild.
Also seems like we haven't documented declaring of async custom properties yet so it might be considered as a private API.

If that's ok I totally support including the new test API to the v1

bgentry · 2018-02-22T21:46:28Z

@bendemboski I was encountering some issues with chaining on collections and with await click() not actually waiting, but then I found out you'd already fixed them 🙏 All working great after the latest commits 🎉

See comment in file

ro0gr · 2018-02-23T20:30:00Z

Thanks to @bendemboski and everyone who took a part here!

san650 · 2018-02-23T20:31:20Z

👏 👏 👏 👏 👏 👏 👏 👏 Amazing work everyone!!!

Thanks @bendemboski for tackling this really difficult task! ❤️

vitch · 2018-04-24T10:13:32Z

addon/-private/helpers.js

@@ -1,9 +1,11 @@
-import Ember from 'ember';
+export { assign } from '@ember/polyfills';


I think this introduced a dependency on a particular version of ember.

In our apps (currently using ember 2.16.2) I got an error when I upgraded to ember-cli-page-object 1.15.0-beta.2 (so that I could use the goodness from this PR):

Could not find module @ember/polyfills imported from ember-cli-page-object/-private/helpers

I can't remember is assign from Ember does anything special but I tried replacing this line with:

export const assign = Object.assign;

locally and it seems to be working...

can you reproduce that in a fresh ember new app? seems strange to me because the ember-cli-babel dependency from the addon should take care of transpiling that correctly 🤔

Thanks for the hint :)

I tried to reproduce it in a fresh ember new app based on the same version of ember-cli and... I can't! See https://github.com/vitch/test-page-object/tree/test-page-object

I'm going to dig further to try and find out what we are doing which is causing problems and I'll let you know if I manage to reproduce it...

bendemboski mentioned this pull request Dec 29, 2017

Support new QUnit modules "acceptance testing" #355

Closed

ro0gr reviewed Dec 30, 2017

View reviewed changes

ro0gr reviewed Dec 31, 2017

View reviewed changes

magistrula reviewed Jan 4, 2018

View reviewed changes

san650 mentioned this pull request Jan 6, 2018

Planning v2 release #359

Closed

18 tasks

magistrula mentioned this pull request Jan 6, 2018

ember-cli-page-object clobbers host app version of ember-test-helpers #361

Closed

bendemboski force-pushed the rfc268 branch from 11b4ea2 to 15dc3d4 Compare January 10, 2018 01:55

magistrula reviewed Jan 10, 2018

View reviewed changes

ro0gr reviewed Jan 10, 2018

View reviewed changes

bendemboski force-pushed the rfc268 branch from 6125cc4 to 69fe50e Compare January 16, 2018 18:43

magistrula reviewed Jan 18, 2018

View reviewed changes

ro0gr reviewed Jan 22, 2018

View reviewed changes

bendemboski force-pushed the rfc268 branch 2 times, most recently from 84ab656 to cc6c733 Compare January 25, 2018 01:41

san650 mentioned this pull request Feb 20, 2018

visit is not defined #380

Closed

bendemboski force-pushed the rfc268 branch from 34f7a5c to 52fd125 Compare February 21, 2018 20:45

Ben Demboski added 10 commits February 21, 2018 13:34

Update to Ember 2.17

467e3ea

Implement RFC268 execution context

4c8fee1

This execution context detects and supports tests using the new QUnit modules API. The tests that rely on chaining behavior are currently failing as the question of if/how to support chaining is unresolved.

Add rfc268 test demonstrating how to test loading behavior

0f6ff25

Support chaining in RFC268 tests

f7f9a62

Perform some trickery (see comment in `create.js`) to allow chained calls to have different async behavior from unchained calls in RFC268 tests.

Run Javascript modules API codemod

8a69cce

Sort out test helpers version mess

518d6a0

Move the RFC268 decision logic to its own file, comment that logic plus the overall execution context switching logic more thoroughly, and remove some commented imports that I forgot to remove before.

Get focus/blur working

bd64b00

Get tests passing in with-ember-test-helpers scenario

1c10d1f

bendemboski force-pushed the rfc268 branch from 52fd125 to 464af45 Compare February 21, 2018 21:34

simonihmig suggested changes Feb 21, 2018

View reviewed changes

ro0gr requested changes Feb 22, 2018

View reviewed changes

Fix wait import

9d565b8

See comment in file

ro0gr approved these changes Feb 23, 2018

View reviewed changes

ro0gr merged commit 9b01f3b into san650:master Feb 23, 2018

vitch reviewed Apr 24, 2018

View reviewed changes

ro0gr mentioned this pull request Nov 20, 2018

feat: add helper for setting a page object in an integration test #432

Closed

ro0gr mentioned this pull request Nov 27, 2018

When will beta be released? #434

Closed

		@@ -1,8 +1,11 @@
		import Ember from 'ember';
		//import { assign, merge } from '@ember/polyfills';

		@@ -1,9 +1,11 @@
		import Ember from 'ember';
		export { assign } from '@ember/polyfills';

Support RFC268 tests #357

Support RFC268 tests #357

Conversation

bendemboski commented Dec 29, 2017 • edited Loading

coveralls commented Dec 29, 2017

ro0gr commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Dec 30, 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 Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0gr Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Dec 30, 2017

coveralls commented Dec 30, 2017

bendemboski commented Dec 31, 2017

Choose a reason for hiding this comment

snewcomer commented Jan 1, 2018

bendemboski commented Jan 1, 2018

coveralls commented Jan 2, 2018

snewcomer commented Jan 2, 2018

ro0gr commented Jan 3, 2018 • edited Loading

bendemboski commented Jan 3, 2018

ro0gr commented Jan 3, 2018

bendemboski commented Jan 3, 2018

magistrula Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jan 4, 2018 • edited Loading

bendemboski commented Jan 10, 2018

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

bendemboski Jan 10, 2018 • 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 left a comment

Choose a reason for hiding this comment

ro0gr Jan 21, 2018 • 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

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

alexlafroscia commented Feb 20, 2018

san650 commented Feb 20, 2018

bendemboski commented Feb 20, 2018

alexlafroscia commented Feb 20, 2018 • edited Loading

bendemboski commented Feb 21, 2018

alexlafroscia commented Feb 21, 2018 • edited Loading

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

Choose a reason for hiding this comment

bendemboski Feb 23, 2018 • 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

bendemboski commented Dec 29, 2017 •

edited

Loading

ro0gr Dec 30, 2017 •

edited

Loading

ro0gr Dec 30, 2017 •

edited

Loading

ro0gr Dec 30, 2017 •

edited

Loading

ro0gr commented Jan 3, 2018 •

edited

Loading

magistrula Jan 4, 2018 •

edited

Loading

coveralls commented Jan 4, 2018 •

edited

Loading

bendemboski Jan 10, 2018 •

edited

Loading

ro0gr Jan 21, 2018 •

edited

Loading

alexlafroscia commented Feb 20, 2018 •

edited

Loading

alexlafroscia commented Feb 21, 2018 •

edited

Loading

bendemboski Feb 23, 2018 •

edited

Loading

ro0gr Feb 22, 2018 •

edited

Loading

bendemboski Feb 22, 2018 •

edited

Loading