cypress-io / cypress

Fast, easy and reliable testing for anything that runs in a browser.
https://cypress.io
MIT License
47.04k stars 3.19k forks source link

Yield object instead of string in cy.wait when request content-type is valid JSON Mime type (not only application/json) #14763

Closed aethr closed 3 years ago

aethr commented 3 years ago

Possibly a duplicate or related to https://github.com/cypress-io/cypress/issues/14273.

Our back-end services use JSON:API spec (https://jsonapi.org/) which recommends the media type application/vnd.api+json. They deliver valid JSON in the response body, which should be converted to an object by cy.wait to allow for deep assertions.

Current behavior

If request is intercepted with cy.intercept(), then cy.wait() yields a string for the response body (related issue: #14273.)

From this comment: https://github.com/cypress-io/cypress/issues/14273#issuecomment-765625317, I believe this is due to the check in https://github.com/cypress-io/cypress/blob/6ee7a9c27e9ea8b7dceee6bae140ce86d816a5e1/packages/driver/src/cy/net-stubbing/events/utils.ts#L4-L20

Due to our back-end sending content-type: application/vnd.api+json this check fails, and so the response body is not converted using JSON.parse.

Desired behavior

I believe that if the response to a request is valid JSON, it should be converted to a JS object regardless of the headers. This approach is taken elsewhere in Cypress, ie: https://github.com/cypress-io/cypress/blob/dae76a8ec0a149d5f4ca4fbe4d15bea1b5f7449d/packages/server/lib/controllers/xhrs.js#L9-L23

Effectively if JSON.parse(text) succeeds without throwing, we can assume that text is valid JSON and return the resulting parsed object.

Test code to reproduce

describe('cy.intercept tests', () => {
  it('test cy.intercept() with application/json', () => {
    cy.intercept('POST', '/users', { foo: 'bar' }).as('postUrl');
    cy.visit('https://example.com');
    cy.then((win) => {
      Cypress.$.ajax({
        url: '/users',
        method: 'POST',
        contentType: 'application/json',
        data: JSON.stringify({ name: 'XYZ' }),
      });
    });

    cy.wait('@postUrl')
      .should((interception) => {
        expect(typeof interception.response.body).to.eq('object');
        expect(interception.response.body).to.deep.eq({ foo: 'bar' })
      });
  });

  it('test cy.intercept() with application/vnd.api+json', () => {
    // temporary API endpoint at mocky.io that responds with 'content-type: application/vnd.api+json'
    cy.intercept('GET', 'https://run.mocky.io/v3/5c528eae-f3c0-4dbe-b9cb-7b12a00320ad').as('jsonApiUrl');
    cy.visit('https://example.com');
    cy.then((win) => {
      Cypress.$.ajax({
        url: 'https://run.mocky.io/v3/5c528eae-f3c0-4dbe-b9cb-7b12a00320ad',
        method: 'GET',
      });
    });

    cy.wait('@jsonApiUrl')
      .should((interception) => {
        expect(interception.response.headers['content-type']).to.contain('application/vnd.api+json');
        // ❗️ fails
        expect(typeof interception.response.body).to.eq('object');
        // ❗️ fails
        expect(interception.response.body).to.deep.eq({ foo: 'bar' });
      });
  });
});

Versions

6.3.0

flotwig commented 3 years ago

Sounds like 'cypress/packages/driver/src/cy/net-stubbing/events/utils.ts' just needs to be updated to detect more valid JSON MIME types.

I don't think we should proactively JSON.parse all incoming request and response bodies, sounds like it could do more harm than good. For example, if you have an endpoint that returns plain text, the type of the response could be object or string depending on if the response happens to be JSON-parseable or not. Keep in mind that true, false, 1, and "foo" are all valid JSON bodies.

aethr commented 3 years ago

Good points. I did some testing and route in Cypress 5 and 6 are both consistent in not converting the response body to objects in this scenario, so my suggestion would probably be a breaking change for some.

A brief search on iana turned up a few likely JSON-compatible media types:

Most seem to follow the same pattern:

Perhaps the check could be expanded to:

return contentType && /^application\/.*json/i.test(contentType)
flotwig commented 3 years ago

@aethr Sounds good. If it gets done by March we can squeeze it into the 7.0.0 release.

github-actions[bot] commented 3 years ago

Internal Jira issue: TR-681

cypress-bot[bot] commented 3 years ago

The code for this is done in cypress-io/cypress#15129, but has yet to be released. We'll update this issue and reference the changelog when it's released.

cypress-bot[bot] commented 3 years ago

Released in 7.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to Cypress v7.0.0, please open a new issue.