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

feat: Use fixture as request body shortcut #27934

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

Crustum7
Copy link
Contributor

@Crustum7 Crustum7 commented Sep 28, 2023

Additional details

The change introduces a dedicated way of using the contents of a fixture as the body of a request message. The name of the fixture is send to the cypress backend and the body of the request is obtained there.

Steps to test

To test the new functionality, use the following types of calls:

cy.request({
  method: 'PUT',
  url: url,
  fixture: 'name'
})
cy.request({ // Should throw an appropriate exception
  url: url,
  fixture: 'name'
})
cy.request({
  url: url,
  method: 'POST',
  fixture: 'name,ascii' // Should result in ASCII encoding
})
cy.request({
  url: url,
  method: 'PUT',
  fixture: 'name',
  body: 'content' // Should throw an appropriate exception
})

How has the user experience changed?

before:

cy.fixture('name').then((data) => {
  cy.request('POST', url, data)
})

after:

cy.request({
  method: 'POST',
  url: url,
  fixture: 'name'
})

PR Tasks

@cypress-app-bot
Copy link
Collaborator

@CLAassistant
Copy link

CLAassistant commented Sep 28, 2023

CLA assistant check
All committers have signed the CLA.

@Crustum7 Crustum7 marked this pull request as ready for review September 29, 2023 07:52
@Crustum7
Copy link
Contributor Author

Crustum7 commented Oct 2, 2023

@AtofStryker @nagash77 could you guys take a look at my PR?

@mschile mschile added the type: feature New feature that does not currently exist label Oct 23, 2023
if (!_.isString(options.url)) {
$errUtils.throwErrByPath('request.url_wrong_type')
}
return requestHandler(...args)
Copy link
Member

Choose a reason for hiding this comment

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

@Crustum7 The refactor included in this change is a great suggestion, but had made it very difficult to review your changes to this file. Can you update to maintain the current file structure so you changes specifically to the command can be reviewed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @emilyrohrbough! Sorry about the messy diff. The problem I'm having is that the request code needs to be called in two different scenarios, normally and after fixture data has been found (in the .then callback). I've therefore taken the request code and put it in a function so I can call it from each of these two places, changing the file structure in the process. I've tried to split the commits in a way that makes the diff less problematic but it doesn't seem to work. Do you have any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is how the code looked before:

Commands.addAll({
    // allow our signature to be similar to cy.intercept
    // METHOD / URL / BODY
    // or object literal with all expanded options
    request (...args) {
      // old code
    },
  })

Here is how it looks now:

Commands.addAll({
    // allow our signature to be similar to cy.intercept
    // METHOD / URL / BODY
    // or object literal with all expanded options
    request (...args) {
      let fxArg = ''

      if (args.length === 2) {
        fxArg = args[1]
        // Default to POST
        if (hasFixturePrefix(fxArg)) {
          args = ['POST', ...args]
        }
      } else if (args.length === 3) {
        fxArg = args[2]
      }

      if (hasFixturePrefix(fxArg)) {
        const fxName = fxArg.split(':')[1]

        return cy.fixture(fxName).then((fixtureData) => {
          args[2] = fixtureData

          return requestHandler(...args)
        })
      }

      return requestHandler(...args)
    },
  })

  const requestHandler = function (...args) {
    // old code (unchanged)
  }

@emilyrohrbough
Copy link
Member

@Crustum7 Thank you for providing a summary of your changes to the request file. This is helpful. With this knowledge, I have taken a closer look at the contribution you are proposing to the cy.request() command. This is a bit long, (sorry!), trying to provide context and details for you.

API Change Requests

I see you have taken the original ticket suggestion of leveraging the string prefix fx: to indicate a fixture should be used. There are a few things to consider with this approach and am suggesting we don't move forward with it:

Observation 1: API signature limits cy.request() usage

What I mean by this is only adding the API signatures of cy.request(url, fixture) and cy.request(method, url, fixture) prevents specifying other cy.request() options like timeout, auth, headers, failOnStatusCode etc.

I would expect to be able to use these option in conjunction to fixture data. This limitation is present given observation 1 listed above where the fixture data is expected to be the request body.

Instead of adding two new API signatures to cy.request(), we should instead add a fixture key to the possible request options supported by the command. This would allow options to be used. Plus it aligns with the existing signature observed in cy.intercept().

cy.request({
  url: '...',
  method: '...',
  fixture: '...'
})

Then, the fixture option can be leveraged whenever the body option is omitted.

cy.request({
  body: '...',
  fixture: '...',
}) // invalid - throw error

Observation 2: currently does not account for specifying the fixture encoding other than the default type

The cy.intercept() commands accepts an comma delineated fixture path and encoding value like fixture,encoding. See here. We can follow this pattern to allow user's to specify the fixture encoding.

cy.request({
  fixture: 'fixture,encoding',
})

Note: this is passed to the backend in the format {fixture:'', encoding: ''} see here

Implementation Notes

From an implementation perspective, we should be able remove calling cy.fixture() within the request command logic, by just passing fixture information to Cypress.backend('http:request', requestOpts) and letting the back-end handle loading the data.

This limits the back and forth communication from the server to the driver to 1 time.

Its a bit tricky to map through this, but Cypress.backend('http:request', ...) is handled here.

This called the onRequest handler, which maps to here.

We should be able to use and/or pass the getFixtures() handler to extract the body. The cy.intercept back-end logic passes the getFixture() handler here for ref.

@Crustum7
Copy link
Contributor Author

Crustum7 commented Nov 8, 2023

Thank you for your feedback @emilyrohrbough! I will try to change the PR to fit with your proposed API. Regarding the implementation details, I have looked in to using Cypress.backend() but ran into the problem of tests for cy.request not expecting more than one call to Cypress.backend(). Should I change the test or create a new test file with a different setup?

@emilyrohrbough
Copy link
Member

@Crustum7 I would update tests if necessary due to implementation changes. You can always push up implementation and request feedback on the direction before doing a full test-revamp. This is a larger change so it could effect existing tests as well as adding new tests.

@Crustum7 Crustum7 marked this pull request as draft November 15, 2023 07:18
@Crustum7 Crustum7 marked this pull request as ready for review November 18, 2023 13:54
@Crustum7
Copy link
Contributor Author

@emilyrohrbough, I have now changed the code to match your suggested changes. The only difference is that the encoding is not passed to the backend as a separate option. Is there a specific reason to pass it in that way?

Also, how can I test the backend code? Do you have any tips?

_.defaults(options, {
headers: {},
gzip: true,
cookies: true,
followRedirect: true,
})

if (options.fixture) {
options.form = await getFixture(options.fixture.filePath, options.fixture.encoding)
Copy link
Member

Choose a reason for hiding this comment

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

My original suggestion was a bit naive with the amount of messaging we are doing in the cy.request() command with assigning headers, validating the body structure etc. Your original approach was definitely better than this suggestion.

What you have here makes assumptions that the fixture data that's for a form, where someone may want it for any body type.

Using the cy.fixture() command will cache the fixture data for the duration fo the spec, which we won't want to introduce, but we can call into the backend directly. something like?

// request.ts
Promise.try(() => {
  if (!options.fixture) return
  return Cypress.backend('get:fixture', options.fixture.filePath, options.fixture.filePath.encoding)`. 
}).then((fixtureData) => {
    options.body = fixtureData
    // handle fixture data
    // handle blob data
}).then(() => {
   return Cypress.backend('http:request', requestOpts)
})
....

I was really hoping to avoid an extra client-side->backend-> exchange with the introduction of this feature.

@chrisbreiding maybe you have some ideas on keeping this a single exchange? maybe its not necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature New feature that does not currently exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use fixture as request body shortcut
5 participants