Request hooks by miherlosev · Pull Request #2200 · DevExpress/testcafe

miherlosev · 2018-03-07T14:48:08Z

Typedefs will be updated in a separate PR.

Feature has 2 parts: in testcafe and in hammerhead.
Firstly, the testcafe part will be reviewed. After that - the hammerhead (DevExpress/testcafe-hammerhead#1527).

For convenience,in this PR the hammerhead part used as module - see https://github.com/DevExpress/testcafe/compare/master...miherlosev:request_hooks?expand=1#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R106.

Changes:

add RequestMock, RequestLogger and RequestHook classes
rename ClientFunctionResultPromise to ReExecutablePromise .
add API for add/removing request hooks on fixture, page and testcontroller levels
add RequestHookConfigureAPIError class
move assertThrow to testing helpers

testcafe-build-bot · 2018-03-07T15:45:40Z

❌ Tests for the commit 107f92f have failed. See details:

testcafe-build-bot · 2018-03-12T13:42:48Z

❌ Tests for the commit 0f0dca7 have failed. See details:

testcafe-build-bot · 2018-03-13T11:03:03Z

❌ Tests for the commit 97d450a have failed. See details:

testcafe-build-bot · 2018-03-13T12:16:00Z

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

testcafe-build-bot · 2018-03-13T16:20:22Z

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

testcafe-build-bot · 2018-03-15T09:18:35Z

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

testcafe-build-bot · 2018-03-15T10:13:17Z

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

miherlosev · 2018-03-15T11:06:16Z

@testcafe-build-bot \retest

miherlosev · 2018-03-15T11:06:49Z

Let's start to review this PR.
/cc @AlexanderMoskovkin @AndreyBelym

testcafe-build-bot · 2018-03-15T12:39:48Z

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

AndreyBelym · 2018-03-16T12:59:06Z

        getActualValueMsg: (value, type) => isNullOrUndefined(value) ? String(value) : type
+    },
+
+    requestHookInheritor: {


I think it's better to call it isRequestHookSubclass.

All conditions are named without is keyword: number, nonNegativeNumber and etc. See more details https://github.com/DevExpress/testcafe/blob/master/src/errors/runtime/type-assertions.js#L10.
But the term subclass is more relevant than inheritor for this context.
Links:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Details_of_the_Object_Model
https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Objects/Inheritance

So, the final name - requestHookSubclass.

AndreyBelym · 2018-03-16T16:10:54Z

+
+    RequestHook,
+
+    ResultPromise,


I feel we need to discuss why this thing is exported.

Motivation:
Requests are asynchronous. It's impossible to check result of custom request hook without adding timeouts. For example, try to remove PromiseResult for this test - https://github.com/DevExpress/testcafe/pull/2200/files#diff-4b5df15c8ad0e18c0978bf61152150ceR33 and try to fix unstable.

It means that customer cannot create a custom request hook with convenient API.

AndreyBelym · 2018-03-16T16:16:50Z

+
+        this.onResponseCallCountInternal = 0;
+
+        Object.defineProperty(this, 'onResponseCallCount', {


You can use ES6 class getter here. Also, IMHO it's better to name it responseHandlerCallCount (and use internalResponseHandlerCallCount for the internal property).

You can use ES6 class getter here

Ok. Let it be.

Also, IMHO it's better to name it responseHandlerCallCount (and use internalResponseHandlerCallCount for the internal property).

It is not handler. It is method from base class. Simular as in Reporter plugin.

Variants: onResponseMethodCallCount, responseCallCount and etc.
So, I will keep it as is.

testcafe-build-bot · 2018-03-19T13:01:07Z

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

testcafe-build-bot · 2018-03-19T17:52:07Z

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

miherlosev · 2018-03-19T21:33:58Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-03-19T22:03:43Z

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

miherlosev · 2018-03-19T22:09:21Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-03-19T22:38:48Z

✅ Tests for the commit 5a0696a have passed. See details:

miherlosev · 2018-03-20T05:39:39Z

FPR

testcafe-build-bot · 2018-03-21T11:53:59Z

✅ Tests for the commit 6fe36e5 have passed. See details:

testcafe-build-bot · 2018-03-22T10:03:30Z

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

testcafe-build-bot · 2018-03-22T12:47:32Z

✅ Tests for the commit 0c61cd6 have passed. See details:

testcafe-build-bot · 2018-03-23T10:36:27Z

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

AlexanderMoskovkin · 2018-03-30T10:05:33Z

+    constructor (name) {
+        super(pageUrl);
+
+        this.name                        = name;


it seems the name field is not used anywhere

AlexanderMoskovkin · 2018-03-30T10:18:13Z

+        await t
+            .click(buttonSelector)
+            .expect(buttonSelector.textContent).eql('Done')
+            .expect(logger.contains(r => r.response.body === browserName)).ok();


add check that other requests (except /get-browser-name) are not saved

AlexanderMoskovkin · 2018-03-30T11:38:28Z

+        });
+    }
+
+    onRequest (/*RequestEvent event*/) {


If this class is abstract this method should throw an error. If a subclass shouldn't do anything here they just should have an empty method

AlexanderMoskovkin · 2018-03-30T11:39:58Z

+            hooks.forEach(hook => {
+                if (this.testRun.requestHooks.includes(hook)) {
+                    remove(this.testRun.requestHooks, hook);
+                    this.testRun._disposeRequestHook(hook);


I propose to encapsulate this login in testRun:

this.testRun.addRequestHook this.testRun.removeRequestHook // usage: hooks.forEach(hook => this.testRun.addRequestHook(hook)); hooks.forEach(hook => this.testRun.removeRequestHook(hook));

AlexanderMoskovkin · 2018-03-30T11:43:30Z

+
+        this.requestHooks = Array.from(this.test.requestHooks);
+
+        this._initRequestHooks();


Do we need that function that contains only one line? I guess inline this.requestHooks.forEach(hook => this._initRequestHook(hook)); call is good.

It's debatable.
Compare two code parts. Which is more readable?

1

this.injectable.scripts.push('/testcafe-automation.js'); this.injectable.scripts.push('/testcafe-driver.js'); this.injectable.styles.push('/testcafe-ui-styles.css'); this.requestHooks = Array.from(this.test.requestHooks); this._initRequestHooks();

2

this.injectable.scripts.push('/testcafe-automation.js'); this.injectable.scripts.push('/testcafe-driver.js'); this.injectable.styles.push('/testcafe-ui-styles.css'); this.requestHooks = Array.from(this.test.requestHooks); this.requestHooks.forEach(hook => this._initRequestHook(hook));

Personally, I Don't see much differences)
In the first case you have to keep in mind that you need to call the _initRequestHooks function after the this.requestHooks = .... In the second case it's obvious from the code.

AlexanderMoskovkin · 2018-03-30T11:44:51Z

+const pageUrl = 'http://localhost:3000/fixtures/api/es-next/request-hooks/pages/index.html';
+const logger  = new RequestLogger(pageUrl);
+
+fixture `RequestLogger`


It can be a unit test.

testcafe-build-bot · 2018-03-30T17:19:12Z

✅ Tests for the commit 443cce4 have passed. See details:

miherlosev · 2018-03-30T17:49:41Z

FPR

AlexanderMoskovkin · 2018-04-02T07:21:54Z

@miherlosev rebase to upstream please

miherlosev · 2018-04-02T09:50:27Z

@miherlosev rebase to upstream please

It will be do after hammerhead version will be updated.

testcafe-build-bot · 2018-04-02T12:30:12Z

❌ Tests for the commit 77d24e3 have failed. See details:

testcafe-build-bot · 2018-04-02T13:35:05Z

✅ Tests for the commit 77d24e3 have passed. See details:

miherlosev · 2018-04-02T13:39:21Z

FPR

miherlosev · 2018-04-02T13:39:35Z

ping @AlexanderMoskovkin @AndreyBelym

miherlosev · 2018-04-02T15:09:44Z

@AndreyBelym Review but don't merge until hammerhead version was updated.

testcafe-build-bot · 2018-04-09T15:34:49Z

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

timwis · 2018-04-10T12:04:42Z

Hi folks - I hope you'll pardon my question, but the fact that xhr request stubbing isn't already provided in testcafe makes me think I may be missing the point or something. Isn't it the case that, without this feature, you can only use testcafe for end-to-end tests? Or is there another way to stub xhr requests? Requiring my tests hit my production backend API will definitely slow things down. Is it, perhaps, about a flag in my application code on NODE_ENV=test and I should swap out the back-end with fixtures/sample data? Or should I use sinon or nock or something?

inikulin · 2018-04-10T17:34:29Z

@timwis You can use proxy between testcafe and origin to redirect XHR requests (see https://devexpress.github.io/testcafe/documentation/using-testcafe/command-line-interface.html#--proxy-host). However, if you want to do that from your test code you'll need this feature.

testcafe-build-bot · 2018-04-16T16:20:35Z

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

testcafe-build-bot · 2018-04-16T19:19:48Z

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

miherlosev · 2018-04-16T19:42:26Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-04-16T20:08:49Z

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

* Request hooks * fix Alex review issues * minor changes * rename 'sessionId ' to 'testRunId' for logged request (RequestLogger) * fix tests * use the current hammerhead version

miherlosev force-pushed the request_hooks branch 2 times, most recently from ba8df34 to cf9a52c Compare March 15, 2018 08:46

miherlosev force-pushed the request_hooks branch from 07b2215 to 6274729 Compare March 15, 2018 09:32

miherlosev changed the title ~~[WIP] Request hooks~~ Request hooks Mar 15, 2018

AndreyBelym suggested changes Mar 16, 2018

View reviewed changes

AlexanderMoskovkin mentioned this pull request Mar 19, 2018

Implement request hooks #1341

Closed

miherlosev force-pushed the request_hooks branch from df26a43 to 5a0696a Compare March 19, 2018 16:07

LavrovArtem mentioned this pull request Mar 21, 2018

[WIP]Add special url flag for the authentication via social networks in iframe DevExpress/testcafe-hammerhead#1539

Closed

AlexanderMoskovkin suggested changes Mar 30, 2018

View reviewed changes

fix Alex review issues

443cce4

minor changes

77d24e3

churkin approved these changes Apr 2, 2018

View reviewed changes

AlexanderMoskovkin approved these changes Apr 2, 2018

View reviewed changes

miherlosev@mail.ru added 2 commits April 9, 2018 15:18

rename 'sessionId ' to 'testRunId' for logged request (RequestLogger)

823535a

fix tests

5b472c8

LavrovArtem mentioned this pull request Apr 9, 2018

Can't interact with websites that require CSRF Tokens #2106

Closed

AndreyBelym approved these changes Apr 10, 2018

View reviewed changes

use the current hammerhead version

eceadab

Merge branch 'master' into request_hooks

f370d01

miherlosev merged commit d7db437 into DevExpress:master Apr 16, 2018

miherlosev deleted the request_hooks branch April 20, 2018 09:20


		this.onResponseCallCountInternal = 0;

		Object.defineProperty(this, 'onResponseCallCount', {


		this.requestHooks = Array.from(this.test.requestHooks);

		this._initRequestHooks();

Conversation

miherlosev commented Mar 7, 2018 • edited by AlexanderMoskovkin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

testcafe-build-bot commented Mar 7, 2018

Uh oh!

testcafe-build-bot commented Mar 12, 2018

Uh oh!

testcafe-build-bot commented Mar 13, 2018

Uh oh!

testcafe-build-bot commented Mar 13, 2018

Uh oh!

testcafe-build-bot commented Mar 13, 2018

Uh oh!

testcafe-build-bot commented Mar 15, 2018

Uh oh!

testcafe-build-bot commented Mar 15, 2018

Uh oh!

miherlosev commented Mar 15, 2018

Uh oh!

miherlosev commented Mar 15, 2018

Uh oh!

testcafe-build-bot commented Mar 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miherlosev Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miherlosev Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

testcafe-build-bot commented Mar 19, 2018

Uh oh!

testcafe-build-bot commented Mar 19, 2018

Uh oh!

miherlosev commented Mar 19, 2018

Uh oh!

testcafe-build-bot commented Mar 19, 2018

Uh oh!

miherlosev commented Mar 19, 2018

Uh oh!

testcafe-build-bot commented Mar 19, 2018

Uh oh!

miherlosev commented Mar 20, 2018

Uh oh!

testcafe-build-bot commented Mar 21, 2018

Uh oh!

testcafe-build-bot commented Mar 22, 2018

Uh oh!

testcafe-build-bot commented Mar 22, 2018

Uh oh!

testcafe-build-bot commented Mar 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

1

2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miherlosev commented Mar 7, 2018 •

edited by AlexanderMoskovkin

Loading

miherlosev Mar 19, 2018 •

edited

Loading

miherlosev Mar 19, 2018 •

edited

Loading

miherlosev commented Apr 2, 2018 •

edited

Loading