Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[docs] Describe request hooks #2309

Merged
merged 34 commits into from
May 14, 2018

Conversation

VasilyStrelyaev
Copy link
Collaborator

\cc @DevExpress/testcafe-docs

@VasilyStrelyaev
Copy link
Collaborator Author

Not quite polished. just the first try, please review

@VasilyStrelyaev
Copy link
Collaborator Author

oh, and I forgot @miherlosev

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 0f5b3ad have passed. See details.

test
.requestHooks(logger)
('test', async t => {
const r = logger.requests[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaningful name for this const is const request = logger.requests[0];. But in this case you have to write ugly code
console.log(request.request.url); . Thus, an array of requests contains no request but logs, otherwise e.g. id would be in request.request.id. Don't you think so? @VasilyStrelyaev @churkin @AlexanderMoskovkin Could we find more suitable name?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the variable name to logRecord

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use all power of es6 and write

const { request } = logger.requests[0];

logRecord is ok for me too.

Property | Type | Description
-------- | ---- | -----------
`id` | String | The ID of the request (generated by the logger).
`testRunId` | String | The ID of the test run that sent the request. Use it to identify the test in which the request originated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should give an example of using testRunId

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt we should expose testRunId. Do you know any cases when a user need it? /cc @miherlosev @VasilyStrelyaev

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miherlosev once said that grouping requests by the origin test is a use case

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the real world case?

await t
            .expect(logRecord.id).eql('UnqLnm189')
            .expect(logRecord.testRunId).eql('IwQA12J12')

It does not make any sense. It will be always failed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ehm, I used to have console.log here instead of assertions, I guess I will go back to them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used to have console.log here instead of assertions

At first please describe why someone need it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need describe

------ | ---- | ------------- | ---------
`logRequestHeaders` | Boolean | Specifies whether the request headers should be logged. | `false`
`logRequestBody` | Boolean | Specifies whether the request body should be logged. | `false`
`stringifyRequestBody` | Boolean | Specifies whether the request body should be stored as a string or an object. | `false`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or an object

which kind of object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the body is just some custom object and that we don't know anything about it (i.e. can't give more details).
So you are saying that we can give more info here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be Buffer

Property | Type | Description
-------- | ---- | -----------
`id` | String | The ID of the request (generated by the logger).
`testRunId` | String | The ID of the test run that sent the request. Use it to identify the test in which the request originated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the real world case?

await t
            .expect(logRecord.id).eql('UnqLnm189')
            .expect(logRecord.testRunId).eql('IwQA12J12')

It does not make any sense. It will be always failed

@testcafe-build-bot
Copy link
Collaborator

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

@testcafe-build-bot
Copy link
Collaborator

✅ Tests for the commit 1c4c8dd have passed. See details.

@testcafe-build-bot
Copy link
Collaborator

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

Copy link
Collaborator

@kirovboris kirovboris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/r-
I think we need to add some simple example(s) to take a look at the feature before the digging into details.

* All TestCafe request hooks descend from the `RequestHook` class.

```js
class MyRequestHook extends RequestHook {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add import statement for RequestHook.

* To determine which requests the hook handles, the base class constructor receives an array of [filtering rules](#specifying-which-requests-are-handled-by-the-hook) as the first parameter. If no rules are passed, all requests are handled.

```js
export default class RequestHook {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export default is an extra noise for this example, just class RequestHook... Also you forgot to close }

constructor (requestFilterRules, responseEventConfigureOpts) {
console.log(responseEventConfigureOpts.includeHeaders); // false
console.log(responseEventConfigureOpts.includeBody); // false
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should close }

* To determine which requests the hook handles, the base class constructor receives an array of [filtering rules](#specifying-which-requests-are-handled-by-the-hook) as the first parameter. If no rules are passed, all requests are handled.

```js
export default class RequestHook {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to describe class interface before detail explanation.

class RequestHook {
    constructor (requestFilterRules, responseEventConfigureOpts) {
        ...
    }

    onRequest (event) {
        ...
    }

    onResponse (event) {
        ...
    }
}


Property | Type | Description
-------- | ---- | ------------
`headers` | Object | The request headers in the property-value form.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These option tables contain a lot of stuff, let's add new item to the table of contents. It should be done for other huge option tables as well.

`port` | Number | The destination port.
`path` | String | The destination path.
`method` | String | The request method.
`credentials` | Object | Credentials that were used to authenticate in the current session using NTLM or Basic authentication. For HTTP Basic authentication, these are `username` and `password`. NTLM authentication additionally specifies `workstation` and `domain`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to describe credentials option in detail or to add a link if it's possible.

* The request's HTTP method.
* The status code received in the response.
* The user agent that sent the request.
* The ID of the test run that sent the request.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs only for customers which will create an own hook. I think we should not describe it.


Property | Type | Description
-------- | ---- | -----------
`id` | String | The ID of the request (generated by the logger).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need describe.

Property | Type | Description
-------- | ---- | -----------
`id` | String | The ID of the request (generated by the logger).
`testRunId` | String | The ID of the test run that sent the request. Use it to identify the test in which the request originated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need describe

### respond

```text
respond(body [, statusCode] [, headers])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body is an optional parameter too.

.onRequestTo(...).respond() returns an empty html page.


Property | Type | Description
-------- | ---- | --------------
`requestId` | String | TestCafe internal request ID.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't describe

`isAjax` | Boolean | Specifies whether this is an AJAX request.
`headers` | Object | The request headers in the property-value form.
`body` | String | A stringified request body.
`testRunId` | String | TestCafe internal ID of the test run that sent the request.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont' describe


### Understanding How TestCafe Request Hooks Operate

* All TestCafe request hooks descend from the `RequestHook` class.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

descend -> inherit

.onRequestTo(/*...*/)
.respond('<html></html>') // an HTML response
.onRequestTo(/*...*/)
.respond(null, 204) // an empty response with a status code
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more case - respond with binary data

.onRequestTo(/*...*/)
.respond(Buffer.from([0x62, 0x75, 0x66, 0x66, 0x65, 0x72]))

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 0118cd1 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit bb73c9b have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

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

@AndreyBelym AndreyBelym added this to the Sprint #11 milestone May 8, 2018
@MargaritaLoseva
Copy link
Contributor

@dirk-pieterse @miherlosev please review

docs/README.md Outdated
@@ -60,6 +60,12 @@
* [Assertion API](articles/documentation/test-api/assertions/assertion-api.md)
* [Obtaining Data from the Client](articles/documentation/test-api/obtaining-data-from-the-client/README.md)
* [Examples of Using Client Functions](articles/documentation/test-api/obtaining-data-from-the-client/examples-of-using-client-functions.md)
* [Intercepting and Mocking HTTP Requests](articles/documentation/test-api/intercepting-and-mocking-http-requests/README.md)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intercepting and Mocking HTTP Requests -> Intercepting HTTP Requests
Mocking is a intercepting too.


If you need to handle HTTP requests in a custom way, you can create your own request hook.

The example below shows a custom hook that adds the `Authorization` header for JWT bearer authorization.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convert to link - JWT


Parameter | Type | Description
--------- | ---- | ------------
`hook` | RequestHook | A request logger, mock or custom hook.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be RequestHook subclass. The build-in: RequestLogger or RequestMock or custom RequestHook written by customer.

So, I propose to change the line as:
hook | RequestHook subclass | A RequestLogger, RequestMock or custom user-defined hook.

--------- | ---- | ------------
`hook` | RequestHook | A request logger, mock or custom hook.

The `requestHooks` methods use the rest operator, which allows you to pass multiple hooks as parameters or arrays of hooks.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence presents twice.
I think we should rewrite as:

The 'hook' parameter of fixture.requestHooks, test.requestHooks, t.addRequestHooks and t.removeRequestHooks is the rest operator.
It allows to pass multiple hooks or arrays of hooks as parameter.

Example

fixture.requestHooks(logger1, logger2, logger3, mock);
test.requestHooks([logger1, logger2, logger3, mock])


The request logger, mock and custom request hooks require that you specify which requests should be handled by them and which should be skipped.

You can perform this filtering by passing the *request filtering rules* to the hook. Note that you can pass a single rule or an array of those.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can setup or You can specify

.respond(responseMock2);
```

To enable the hook to mock the requests, [attach it to a test or fixture](attaching-hooks-to-tests-and-fixtures.md).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enable the hook to mock the requests - it's unnecessary phrase.
Simplify to 'Then attach it to a test or fixture.' Or something like that.

The common request hook working scheme: firstly - configure, secondary - attach to fixture or test.


Parameter | Type | Description | Default
--------- | ---- | ------------- | -----
`body`&#160;*(optional)* | Object &#124; String &#124; Function | A mocked response body. Pass an object for a JSON response, a string for an HTML response or a function to build a custom response. | An empty HTML page is returned with the response.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add Buffer type for body too.

--------- | ---- | ------------- | -----
`body`&#160;*(optional)* | Object &#124; String &#124; Function | A mocked response body. Pass an object for a JSON response, a string for an HTML response or a function to build a custom response. | An empty HTML page is returned with the response.
`statusCode`&#160;*(optional)* | Number | The response status code. | `200`
`headers`&#160;*(optional)* | Object | Custom headers added to the response in the property-value form.| The `content-type` header is provided.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extend this:
If content-type is specified then its value will be used.

If content-type header is not specified then it will be calculated according the body type.
If body is object then content-type will be application/json.
if body has another type then content-type will be text/html; charset=utf-8.


Parameter | Type | Description
--------- | ---- | ---------------
`req` | Object | A request to be mocked.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is request options from https://github.com/DevExpress/testcafe/pull/2309/files#diff-10e0cabf319ff29f01739d995e6ad73aR121.
Maybe need to place in separate paragraph or page?


Method | Description
------ | ---------------
`setBody(value)` | Sets the `value` as the response body.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value parameter should be String.

@testcafe-build-bot
Copy link
Collaborator

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

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 90e7823 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 90e7823 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit b40ff58 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit b40ff58 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 4fb0c47 have failed. See details.


Parameters | Type | Description | Default
---------- | ---- | ----------- | -----
`filter`&#160;*(optional)* | String &#124; RegExp &#124; Object &#124; Predicate | Specifies which requests should be mocked with a response that follows in the `respond` method. See [Specifying Which Requests are Handled by the Hook](specifying-which-requests-are-handled-by-the-hook.md). | All requests are mocked.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filter is required parameter


Parameter | Type | Description | Default
--------- | ---- | ------------- | -----
`body`&#160;*(optional)* | Object &#124; String &#124; Function &#124; Buffer | A mocked response body. Pass an object for a JSON response, a string for an HTML response or a function to build a custom response. | An empty HTML page is returned with the response.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add link to the Buffer type

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit bf3f3f7 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 6efaa50 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

❌ Tests for the commit 2c80769 have failed. See details.

@testcafe-build-bot
Copy link
Collaborator

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

@MargaritaLoseva
Copy link
Contributor

@kirovboris @miherlosev fpr

@@ -60,6 +60,12 @@
* [Assertion API](articles/documentation/test-api/assertions/assertion-api.md)
* [Obtaining Data from the Client](articles/documentation/test-api/obtaining-data-from-the-client/README.md)
* [Examples of Using Client Functions](articles/documentation/test-api/obtaining-data-from-the-client/examples-of-using-client-functions.md)
* [Intercepting HTTP Requests](articles/documentation/test-api/intercepting-and-mocking-http-requests/README.md)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the filename too
articles/documentation/test-api/intercepting-http-requests/README.md

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't convenient to perform this from Vasily's fork.
I'll rename this topic and fix links to it from my fork in a separate PR after merging this PR.

@MargaritaLoseva MargaritaLoseva merged commit ac06c5d into DevExpress:master May 14, 2018
@VasilyStrelyaev VasilyStrelyaev deleted the request-hooks branch May 17, 2018 14:40
kirovboris pushed a commit to kirovboris/testcafe-phoenix that referenced this pull request Dec 18, 2019
* Describe request hooks

* Address Helen's remarks

* Revert an example

* Specify the body type - Buffer

* Change structure and address remarks

* Add real-life examples

* Update TOC

* Fix broken links

* Update README.md

* Update README.md

* Update README.md

* Update attaching-hooks-to-tests-and-fixtures.md

* Update creating-a-custom-http-request-hook.md

* Update logging-http-requests.md

* Update mocking-http-responses.md

* Update specifying-which-requests-are-handled-by-the-hook.md

* fix Mikhail's remark

* fix Mikhail's remarks

* attaching hooks - fix Mikhail's remarks

* specifying-which-requests-are-handled -Fix remarks

* creating-a-custom-http-request-hook - Fix remarks

* logging-http-requests -- fix Mikhail's remark

* remove Mocking from header

* Remove Mocking from header

* creating-a-custom-http-request-hook Fix remarks

* mocking-http-responses Fix Mikhail's remarks

* formatting added

* remove trailing-spaces

* Remove trailing spaces

* mocking-http-responses.md Fix Mikhail's remarks

* Remove trailing spaces

* return previous permalink for Readme.md

* return previous permalink for request hooks main topic

* fix broken link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants