Skip to content

Conversation

@colleenmcginnis
Copy link
Contributor

@colleenmcginnis colleenmcginnis commented Aug 9, 2022

Closes #2049

Overview

Adds documentation on the new request parameter that allows users to make API requests independently of the browser interactions. For example, to get authentication credentials or tokens in service of a browser-based test.

For the reviewer

@paulb-elastic @andrewvc:

  • Does it make sense to mention this initially when introducing journey?
  • Does it make sense to add the examples to Working with params and secrets? From what I've read I think requesting credentials/tokens is an important use case, but I'm not sure if it makes sense to also document this functionality outside the context of params/secrets.

To do

@mergify
Copy link
Contributor

mergify bot commented Aug 9, 2022

This pull request does not have a backport label. Could you fix it @colleenmcginnis? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-/d./d is the label to automatically backport to the /d./d branch. /d is the digit
    NOTE: backport-skip has been added to this pull request.

@ghost
Copy link

ghost commented Aug 9, 2022

A documentation preview will be available soon:

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Aug 9, 2022
@colleenmcginnis colleenmcginnis added backport-8.4 Automated backport with mergify and removed backport-skip Skip notification from the automated backport with mergify labels Aug 10, 2022
@paulb-elastic
Copy link
Contributor

@colleenmcginnis

Does it make sense to add the examples to Working with params and secrets?

I agree, it seems a little odd for it to go in the params and secrets section as it's out of context for the rest of that page.

I think it might be better on the write a test page. I think the reference to request you've already got there makes sense too, but maybe move the block from here to that page (maybe between add steps and setup global state)?.

@colleenmcginnis colleenmcginnis changed the title Document the request parameter [synthetics] Document the request parameter Aug 10, 2022
Copy link
Contributor Author

@colleenmcginnis colleenmcginnis left a comment

Choose a reason for hiding this comment

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

This is ready for review -- a few questions/comments below.

@colleenmcginnis colleenmcginnis marked this pull request as ready for review August 15, 2022 14:47
@colleenmcginnis colleenmcginnis requested a review from a team as a code owner August 15, 2022 14:47
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

I like the new docs but found some bugs!

Copy link
Contributor

@paulb-elastic paulb-elastic left a comment

Choose a reason for hiding this comment

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

Agree that the example maybe does look a bit complex as there are other things in there about decoding that aren’t necessarily related to the request.

If the purpose of the docs is to show how a request can be made and they used in the script (irrespective of having an example in our demos are at some point), maybe a simpler example would be useful.

I’ll defer to @andrewvc’s suggestions here, but here is an example I put together for something recently (it makes a request for a quote of the day, then uses this in a Google search):

step("Google Page", async () => {
  // HTTP request for the quote
  const resp = await  request.get("http://quotes.stormconsultancy.co.uk/random.json");
  // parse the response
  quote = JSON.parse((await resp.body()).toString()).quote;
  // visit Google with the quote
  await page.goto('https://www.google.com?q=' + quote);
});

(Maybe this is too simple and not a realistic enough scenario, so feel free to use it or not)

I'll LGTM and defer to Andrew's suggestion regarding the specifics of the example.

Comment on lines 195 to 198
// The Elastic Synthetics `request` parameter is similar to Playwright's
// https://playwright.dev/docs/api/class-apirequestcontext[`APIRequestContext`], but
// it comes built into Elastic Synthetics and doesn't have to be imported separately.
// This reduces the code needed and allows you to make API requests in inline journeys.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to mention how this is different from the other request objects that is exposed by Playwright.

@vigneshshanmugam is this what you had in mind? Are there other differences?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something along the lines of

  1. Top level request object exposed by synthetics have its own isolated cookie storage and its different from the context.request and page.request as they shares cookie storage with the corresponding BrowserContext.
  2. If you want to control the creation of this request object - https://playwright.dev/docs/api/class-apirequest#api-request-new-context, you can do so by passing options via --playwright-options or synthetics.config.ts file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @vigneshshanmugam. How does this look? 0fc3fee

Here's what the whole section looks like:

Screen Shot 2022-08-23 at 2 28 25 PM

Copy link
Member

Choose a reason for hiding this comment

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

Looks great, Thanks @colleenmcginnis

@colleenmcginnis colleenmcginnis self-assigned this Aug 23, 2022
Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

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

LGTM

@colleenmcginnis colleenmcginnis dismissed andrewvc’s stale review August 23, 2022 22:05

I'm moving forward with other approvals -- I can make additional changes as needed in a follow-up PR.

@colleenmcginnis colleenmcginnis merged commit ed30777 into elastic:main Aug 23, 2022
@colleenmcginnis colleenmcginnis deleted the issue-2049 branch August 23, 2022 22:27
mergify bot pushed a commit that referenced this pull request Aug 23, 2022
* initial approach

* move from params/secrets to write a test

* remove detailed examples

* add info on how synthetics request is different than playwright request

* final clean up

(cherry picked from commit ed30777)
colleenmcginnis added a commit that referenced this pull request Aug 24, 2022
* initial approach

* move from params/secrets to write a test

* remove detailed examples

* add info on how synthetics request is different than playwright request

* final clean up

(cherry picked from commit ed30777)

Co-authored-by: Colleen McGinnis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-8.4 Automated backport with mergify v8.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Document Request Object

4 participants