Implemented DOM Node extensions mechanism (closes #749) #795

kirovboris · 2016-09-13T11:03:26Z

\cc @inikulin @AlexanderMoskovkin @VasilyStrelyaev

testcafe-build-bot · 2016-09-13T11:33:10Z

✅ Tests for the commit b15426e have passed. See details.

inikulin · 2016-09-13T11:43:06Z

Wow, it was quick, but unfortunately not on time. We're on code freeze right now, so let's merge it after release, ok? Also, we have a new rule of adding/modifying basic example tests to demonstrate new feature. Can you add such example for find method (for extensions we'll have dedicated topic in docs about selector extensions)

kirovboris · 2016-09-13T11:48:40Z

let's merge it after release, ok?

Of course, I'll switch on the actual tasks now and will add an example later.

VasilyStrelyaev · 2016-09-13T11:54:05Z

lgtm, meanwhile

testcafe-build-bot · 2016-11-25T10:56:23Z

✅ Tests for the commit cc2f714 have passed. See details:

testcafe-build-bot · 2016-11-28T07:46:26Z

✅ Tests for the commit e1325a5 have passed. See details:

kirovboris · 2016-12-05T07:55:03Z

@inikulin @AlexanderMoskovkin
In according to the last API changes, i suggest the following usage of snapshot extensions:

const el1 = Selector('.my-class').extendSnapshot({
    prop1: node => ...
});

expect(await el1.prop1).eql(...); 

const el2 = el1.extendSnapshot({
    prop2: node => ...
})

expect(await el2.prop1).eql(...); 
expect(await el2.prop2).eql(...);

I.e. we add .extendSnapshot method instead of .with({snapshotExtensions: {...} }) and provide possibility its hierarchical using.

testcafe-build-bot · 2016-12-09T07:23:05Z

❌ Tests for the commit 5e34753 have failed. See details:

kirovboris · 2016-12-09T08:00:28Z

@testcafe-build-bot \retest

testcafe-build-bot · 2016-12-09T08:26:11Z

❌ Tests for the commit 5e34753 have failed. See details:

kirovboris · 2016-12-09T08:42:31Z

@testcafe-build-bot \retest

testcafe-build-bot · 2016-12-09T09:26:19Z

❌ Tests for the commit 5e34753 have failed. See details:

inikulin

\r-

inikulin · 2016-12-09T09:38:11Z

src/client-functions/selector-builder/add-api.js

+}
+
+function addSnapshotPropertyShorthands (obj, getSelector, snapshotExtensions) {
+    var properties = SNAPSHOT_PROPERTIES.concat(snapshotExtensions ? Object.keys(snapshotExtensions) : []);


I guess it's better be rewritten as:

var properties = SNAPSHOT_PROPERTIES; if (snapshotExtensions) properties = properties.concat(Object.keys(snapshotExtensions));

inikulin · 2016-12-09T09:41:26Z

src/client-functions/selector-builder/add-api.js

@@ -223,7 +237,17 @@ function addFilterMethods (obj, getSelector, SelectorBuilder) {
    };
 }

-function createHierachicalSelector (getSelector, SelectorBuilder, selectorFn, filter, additionalDependencies) {
+function addSnapshotExtensionMethods (obj, getSelector, SelectorBuilder) {
+    obj.extendSnapshot = extensions => {


We are getting rid of snapshot term in the first place. Maybe we'll rename it to addCustomDomProperty?

inikulin · 2016-12-09T09:42:41Z

src/client-functions/selector-builder/add-api.js

+        assertObject('extendSnapshot', '"extendSnapshot" option', extensions);
+
+        Object.keys(extensions).forEach(prop => {
+            assertFunction('extendSnapshot', `extendSnapshot '${prop.replace(/'/g, "\\'")}'`, extensions[prop]);


Why we need to escape ' in property name here? Also I guess it's better to change text to Snapshot extensions method or smthg like that

inikulin · 2016-12-09T09:45:56Z

src/client/driver/command-executors/client-functions/selector-executor/index.js

@@ -40,6 +41,12 @@ export default class SelectorExecutor extends ClientFunctionExecutor {
        }
    }

+    _extendSnapshot (snapshot, node, extensions) {
+        Object.keys(extensions).forEach(prop => {
+            snapshot[prop] = extensions[prop](node);


It would be good to catch errors from extension methods and report them appropriately, e.g.:

`someProp` custom selector DOM property error: [error text]

inikulin · 2016-12-09T09:52:11Z

src/client/driver/command-executors/client-functions/selector-executor/index.js

+            .getResult()
+            .then(result => {
+                var node               = result;
+                var snapshot           = this.replicator.encode(result)[0];


We need to pass extensions to replicator and appropriate transformers somehow. If you attach them afterwards, newly added properties will not be processed by replicator and thus circular references will throw errors and you will not be able to encode advanced data types. Also, note that selector can work in counter and collection modes: https://github.com/kirovboris/testcafe-phoenix/blob/5e34753e68f732c0ad639f9076c349330422014f/src/client/driver/command-executors/client-functions/selector-executor/filter.js#L101 First one used to calculate .exists and .count and the second one is used in chains. Make sure everything works correctly with extensions in this mode.

testcafe-build-bot · 2016-12-09T10:02:36Z

❌ Tests for the commit 5e34753 have failed. See details:

testcafe-build-bot · 2016-12-23T14:00:14Z

❌ Tests for the commit b63632c have failed. See details:

kirovboris · 2016-12-23T14:44:56Z

@testcafe-build-bot \retest

testcafe-build-bot · 2016-12-23T15:13:12Z

❌ Tests for the commit b63632c have failed. See details:

kirovboris · 2016-12-23T15:25:14Z

@testcafe-build-bot \retest

testcafe-build-bot · 2016-12-23T15:56:35Z

✅ Tests for the commit b63632c have passed. See details:

kirovboris · 2016-12-24T07:16:12Z

FPR

VasilyStrelyaev · 2016-12-26T09:30:06Z

As far as I understand your recent changes in the docs, we now use the term snapshot for an object that is explicitly awaited, When talking about DOM node properties accessed by using a selector in general, we say state.

Since custom properties can also be accessed through shorthands, we may need to rename the method to extendState or simply extend.

Does it make sense?

kirovboris · 2016-12-26T09:37:01Z

Yes, i agree, also extend is shorter for typing.

AlexanderMoskovkin · 2016-12-28T11:22:24Z

src/client-functions/selector-builder/add-api.js

@@ -244,7 +262,17 @@ function addFilterMethods (obj, getSelector, SelectorBuilder) {
    };
 }

-function addHierachicalSelectors (obj, getSelector, SelectorBuilder) {
+function addCustomDomProperty (obj, getSelector, SelectorBuilder) {


I think it's wrong naming.

It should be what we are adding to selector object (as addFilterMethods). So, it should be like addExtendMethod.

inikulin

\r-

inikulin · 2016-12-29T13:29:45Z

src/client-functions/selector-builder/add-api.js

@@ -88,8 +90,23 @@ async function getSnapshot (getSelector, callsite) {
    return node;
 }

-function addSnapshotPropertyShorthands (obj, getSelector) {
-    SNAPSHOT_PROPERTIES.forEach(prop => {
+function assertSnapshotExtensionOptions (extensions) {


I've discussed it with @VasilyStrelyaev, he didn't see addCustomDOMProperties variant. Let's stick with it. Also, make sure that naming is consistent across codebase

inikulin · 2016-12-29T13:33:38Z

src/errors/test-run/templates.js

@@ -78,6 +78,12 @@ export default {
        ${escapeHtml(err.errMsg)}
    `),

+    [TYPE.uncaughtErrorInSnapshotExtensionCode]: err => markup(err, `
+        An error occurred when trying to calculate a custom ${err.property} property:


Custom property of what?

Of selector? Don't think so. I have no other thoughts though. Custom DOM element state property?

@kirovboris, whose property is this?

Should we write something like "... custom Selector property <prop>"?
@inikulin

@kirovboris yes

inikulin · 2016-12-29T13:35:30Z

src/client/driver/command-executors/client-functions/client-function-executor.js

-            new FunctionTransform()
-        ]);
+    //Overridable methods
+    _addReplicatorTransform () {


No need to extract methods this way for replicator, just keep it as is. In selector constructor just create SelectorTransform and add it to replicator manually.

I'm not sure I understood your comment. If I add ClientFunctionTransform in constructor of the base class ClientFunctionExecutor, I can't add SelectorTransform in SelectorExecutor constructor, because we can't add two transforms with the same type Node to the replicator.

inikulin · 2016-12-29T13:35:49Z

test/functional/fixtures/api/es-next/selector/testcafe-fixtures/selector-test.js

+            prop2: node => 'tagName: ' + node.tagName
+        });
+
+    expect(await el.prop1).eql(42);


Use built-in assertons

testcafe-build-bot · 2017-01-09T12:16:26Z

✅ Tests for the commit 541d615 have passed. See details:

kirovboris · 2017-01-09T12:48:45Z

FPR, we've discussed with @VasilyStrelyaev the name of method and decided to keep .addCustomDOMProperties()

inikulin

Overall, lgtm. Just some minor issues left. Also, would you mind adding feature summary?

inikulin · 2017-01-10T09:00:29Z

src/client/driver/command-executors/client-functions/replicator.js

-        this.type = 'Node';
+    constructor (customDOMProperties) {
+        this.type       = 'Node';
+        this.customDOMProperties = customDOMProperties || {};


Formatting issue, assignments should be aligned

inikulin · 2017-01-10T09:01:50Z

src/errors/test-run/templates.js

@@ -78,6 +78,12 @@ export default {
        ${escapeHtml(err.errMsg)}
    `),

+    [TYPE.uncaughtErrorInCustomDOMPropertyCode]: err => markup(err, `
+        An error occurred when trying to calculate a custom Selector property ${err.property}:


custom Selector DOM property? I guess it's worth enclosing property name into quotes: "${err.property}"

inikulin · 2017-01-10T09:02:36Z

src/errors/runtime/type-assertions.js

@@ -59,6 +59,13 @@ export function assertNonNullObject (callsiteName, what, value) {
    }
 }

+export function assertFunction (callsiteName, what, value) {


It should be implemented already, try to rebase onto latest changes

kirovboris · 2017-01-10T10:37:06Z

So, we can extend a snapshot / Selector the following way:

//Add the new property 'contentEditable' to the Selector
const el = Selector('.my-el').addCustomDOMProperties({
    contentEditable: node => node.contentEditable
});

//Now, we can use it as shorthand property 
await t.expect(await el.contentEditable).eql(...);

//Or as a snapshot's property
const snapshot = await el();

await t.expect(await snapshot.contentEditable).eql(...);

//Also we can extend / override custom properties
el = el.addCustomDOMProperties({
    contentEditable: node => /*some_new_logic*/,
    tabIndex: node => node.tabIndex
});

kirovboris · 2017-01-10T10:37:24Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-01-10T10:38:20Z

❌ Tests for the commit 53d31bb have failed. See details:

kirovboris · 2017-01-10T10:42:19Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-01-10T11:14:07Z

✅ Tests for the commit 53d31bb have passed. See details:

kirovboris · 2017-01-10T11:15:58Z

FPR

testcafe-build-bot · 2017-01-10T13:04:00Z

❌ Tests for the commit 04bd388 have failed. See details:

kirovboris · 2017-01-10T13:05:37Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-01-10T13:37:41Z

✅ Tests for the commit 04bd388 have passed. See details:

kirovboris · 2017-01-10T13:39:30Z

FPR

inikulin

lgtm

…vExpress#795) * Implemented DOM Node extensions mechanism (closes DevExpress#749) * Fixed remarks * Renamed extendSnapshot method * Fixed remark * Fixed remarks * Fixed remarks * Added the possibility of extending non ElementNode snapshots

inikulin mentioned this pull request Sep 13, 2016

Redesign a selector's system #798

Closed

22 tasks

kirovboris force-pushed the extending_node_snapshot branch from b15426e to cc2f714 Compare November 25, 2016 10:19

kirovboris force-pushed the extending_node_snapshot branch from cc2f714 to e1325a5 Compare November 28, 2016 06:49

kirovboris changed the title ~~Implemented DOM Node extensions mechanism, added 'find' method (closes #749)~~ Implemented DOM Node extensions mechanism (closes #749) Nov 28, 2016

kirovboris force-pushed the extending_node_snapshot branch from e1325a5 to 5e34753 Compare December 9, 2016 06:57

kirovboris added this to the Sprint #3 milestone Dec 9, 2016

inikulin suggested changes Dec 9, 2016

View reviewed changes

kirovboris removed this from the Sprint #3 milestone Dec 9, 2016

kirovboris force-pushed the extending_node_snapshot branch from 5e34753 to b63632c Compare December 23, 2016 12:59

VasilyStrelyaev approved these changes Dec 28, 2016

View reviewed changes

AlexanderMoskovkin suggested changes Dec 28, 2016

View reviewed changes

inikulin suggested changes Dec 29, 2016

View reviewed changes

kirovboris force-pushed the extending_node_snapshot branch 2 times, most recently from ed40d0c to 541d615 Compare January 9, 2017 11:30

inikulin suggested changes Jan 10, 2017

View reviewed changes

kirovboris added 6 commits January 10, 2017 12:15

Implemented DOM Node extensions mechanism (closes DevExpress#749)

4d72832

Fixed remarks

46a186a

Renamed extendSnapshot method

93a0a1e

Fixed remark

093e115

Fixed remarks

5acdeaf

Fixed remarks

53d31bb

kirovboris force-pushed the extending_node_snapshot branch from 541d615 to 53d31bb Compare January 10, 2017 10:04

Added the possibility of extending non ElementNode snapshots

04bd388

inikulin approved these changes Jan 10, 2017

View reviewed changes

AlexanderMoskovkin approved these changes Jan 11, 2017

View reviewed changes

AlexanderMoskovkin merged commit 7d9da93 into DevExpress:master Jan 11, 2017

Implemented DOM Node extensions mechanism (closes #749) #795

Implemented DOM Node extensions mechanism (closes #749) #795

Conversation

kirovboris commented Sep 13, 2016

testcafe-build-bot commented Sep 13, 2016

inikulin commented Sep 13, 2016

kirovboris commented Sep 13, 2016

VasilyStrelyaev commented Sep 13, 2016

testcafe-build-bot commented Nov 25, 2016

testcafe-build-bot commented Nov 28, 2016

kirovboris commented Dec 5, 2016

testcafe-build-bot commented Dec 9, 2016

kirovboris commented Dec 9, 2016

testcafe-build-bot commented Dec 9, 2016

kirovboris commented Dec 9, 2016

testcafe-build-bot commented Dec 9, 2016

inikulin 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

testcafe-build-bot commented Dec 9, 2016

testcafe-build-bot commented Dec 23, 2016

kirovboris commented Dec 23, 2016

testcafe-build-bot commented Dec 23, 2016

kirovboris commented Dec 23, 2016

testcafe-build-bot commented Dec 23, 2016

kirovboris commented Dec 24, 2016

VasilyStrelyaev commented Dec 26, 2016

kirovboris commented Dec 26, 2016

Choose a reason for hiding this comment

inikulin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VasilyStrelyaev Dec 30, 2016 • edited

Choose a reason for hiding this comment

kirovboris Dec 30, 2016 • edited

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

testcafe-build-bot commented Jan 9, 2017

kirovboris commented Jan 9, 2017

inikulin 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

kirovboris commented Jan 10, 2017 • edited

kirovboris commented Jan 10, 2017

testcafe-build-bot commented Jan 10, 2017

kirovboris commented Jan 10, 2017

testcafe-build-bot commented Jan 10, 2017

kirovboris commented Jan 10, 2017

testcafe-build-bot commented Jan 10, 2017

kirovboris commented Jan 10, 2017

testcafe-build-bot commented Jan 10, 2017

kirovboris commented Jan 10, 2017

inikulin left a comment

Choose a reason for hiding this comment

VasilyStrelyaev Dec 30, 2016 •

edited

kirovboris Dec 30, 2016 •

edited

kirovboris commented Jan 10, 2017 •

edited