-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
more permissive check for json to include application/vnd.api+j… #5166
Conversation
There's a test for the existing cypress/packages/server/test/unit/request_spec.coffee Lines 438 to 450 in 8a9ec90
|
Also, I didn't know there was this much controversy about what the correct JSON MIME type is: |
@flotwig thank you! I can't tell why some of the tests on this are failing. Specifically, |
Looking at 3_user_agent_spec, there's this failure: It seems like it's no longer converting JSON according to the same rules, although I'm not sure exactly how your changes caused this, since it looks correct to me. |
@flotwig Poking around more, I see some other "deeply equal" errors that seem to hinge on seemingly trivial formatting differences for specs that pass when not run e2e. For example,
...but it's just running |
@flotwig I'm a bit embarrassed by this solution but all the tests pass. I don't really know coffeescript so if there's a more elegant way to do this without causing all the tests to fail, I'd love to know more. As is, I've added a test for responses with a |
Got it figured out by setting Re: the e2e tests - And "seem to hinge on seemingly trivial formatting differences for specs that pass" - the reason you see a formatting difference, is that it's expecting a JS object, but it's getting a JS string (JSON). So it might look like just a small formatting difference in the error, but it's actually a completely different object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once tests pass
* more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON
* fix specs * use debugger protocol for cookie handling in electron * use latest gulp * use rimraf instead of gulp-clean * use electron 3.1.8 and node 10.2.1 * use gulp 4 in packages/static * fix sendCommandAsync, log Schema.getDomains on CDP connect * autofill e2e test name [skip ci] * electron@5.0.7, see what new failures exist * --no-sandbox for launching Electron * update cookies logic for electron * node 12 * update snapshot for new node * update error message for new node * stub sendCommandAsync * only connect to socket if path has been replaced, fixes #4776 * update node-sass to support node 12 * skip wacky socket tests for now * snapshot * fix run_plugins_spec snapshot, don't include stack trace * use --no-sandbox on linux to run as root * allow sendCommandAsync to resolve * use euid for root check * log domains even if undefined * don't worry about ending 1xx responses immediately anymore * use --max-http-header-size, change max size from 8kb to 1mb, fix #76 * do not send 502 on failed websocket, just send back ECONNRESET * update websocket spec port to not collide with other test * update outdated expect * Revert "only connect to socket if path has been replaced, fixes #4776" This reverts commit f179eda. * update gulp in root * update https-proxy unit tests * update network spec to properly close server * update reporter spec * update https-proxy-agent to fix node 10.10.0 change discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799 * only pass --max-http-header-size on node >=12 * use own server-destroy implementation that supports secureConnect events * oops * update socket_spec * electron 6.0.0 * console.table introduced in node 10 * change browserify entry to init.js * handle edge case when no response body * console.table added in node 10 * do not exit app when all BrowserWindows are closed * update e2e snapshots * value may not be null * update plugins spec * correct cookie expiry, use browser.getversion for CDP version check * fix snapshotting for require stacks * reorder cookies in spec * warn when depreated electron callback apis are used * only report 1 plugin error per process * cleanup * node 12.4.0, cypress/browsers:node12.4.0-chrome76 docker image * update shell.openExternal to promisified * update dialog.showOpenDialog to promisified * update webContents.session.setProxy to promisified * updating native dependencies since we don't need ancient node ABI support anymore * WIP: switch cookies to simpler, jar-less approach * WIP: switch cookies to simpler, jar-less approach * making tests pass * improve cookie filtering logic * Remove unneeded Promise.try * filter what makes it to the extension * properly re-set superdomain cookies on cross-origin cy.visit * allow comma-separated list of e2e tests * sort cookies in order of expiration date, ascending * updating tests, cleanup * update tests * version electron as a devDependency, electron@6.0.1 * cleanup, remove old automation * cleanup, remove old automation * bump chokidar to fix win10 + node12 issue was seeing this on windows: nuxt/nuxt#6035 fixed with version bump * enable now-supported quit role, re-enable old tests * don't need that arg there * remove last deprecated callback electron invocations * Delete cypress.json * responding to PR feedback * cleanup * invoke * use 'quit' role * Use new appMenu role for Cypress menu on mac * electron@6.0.2 * electron@6.0.3 * remove domain: cookie.domain and see what happens * remove setErrorHandler * Revert "remove domain: cookie.domain and see what happens" This reverts commit 49e9168. * add unit tests for cookies * ci * fix project-content css * electron@6.0.4 * fix specs_list test * electron@6.0.7 * some cleanup * electron@6.0.9 * Update 8_reporters_spec.coffee.js * electron@5.0.10 - Chromium 73, Node 12 * cli: fix the STDIN pipe on Windows (#5045) * cli: pipe stdin * uggh, here is the actual change * update cli unit tests * add unit test * more permissive check for json to include application/vnd.api+j… (#5166) * more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON * fix eslint for fixture specs (#5176) * update eslint to lint files within 'fixtures' in support files - ignore some edge cases like jquery, jsx and obvious js files we wrote with broken code * Fixes from eslint to 'fixtures' files * Catch env variable with reserved name CYPRESS_ENV 1621 (#1626) * server: check CYPRESS_ENV variable when merging configs * catch invalid CYPRESS_ENV value in CLI, close #1621 * linting * sanitize platform in test snapshot * linting * update error message text * add missing comma * fix finally merge in JS code * pass CLI linter * fix log reference, should be debug * use correct sinon reference * update message, show first part in red * update error message text * Addresses #2953 (#5174) * Addresses #2953 * Added proper test for new error message * Didn't realize it ran this test as well, whoops * Implementing changes as suggested by @jennifer-shehane * Fixing tests and error output. Moved the checks to the start of the get command to ensure we always catch improper options * Removing issue test since the querying spec covers it * Using coffescript isArray check * depromisify things that were promisified b/t electron 5 <=> 6 Revert "update shell.openExternal to promisified" This reverts commit 8b6460d. Revert "update dialog.showOpenDialog to promisified" This reverts commit 5f178b0. Revert "update webContents.session.setProxy to promisified" This reverts commit 727df3a. * node12.4.0-chrome76 => node12.0.0-chrome75 * fix tests for electron downgrade * node12.0.0-chrome75 => node12.0.0-chrome73 Co-authored-by: Zach Bloomquist <github@chary.us> Co-authored-by: Brian Mann <brian.mann86@gmail.com>
…ress-io#5166) * more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON
* fix specs * use debugger protocol for cookie handling in electron * use latest gulp * use rimraf instead of gulp-clean * use electron 3.1.8 and node 10.2.1 * use gulp 4 in packages/static * fix sendCommandAsync, log Schema.getDomains on CDP connect * autofill e2e test name [skip ci] * electron@5.0.7, see what new failures exist * --no-sandbox for launching Electron * update cookies logic for electron * node 12 * update snapshot for new node * update error message for new node * stub sendCommandAsync * only connect to socket if path has been replaced, fixes cypress-io#4776 * update node-sass to support node 12 * skip wacky socket tests for now * snapshot * fix run_plugins_spec snapshot, don't include stack trace * use --no-sandbox on linux to run as root * allow sendCommandAsync to resolve * use euid for root check * log domains even if undefined * don't worry about ending 1xx responses immediately anymore * use --max-http-header-size, change max size from 8kb to 1mb, fix cypress-io#76 * do not send 502 on failed websocket, just send back ECONNRESET * update websocket spec port to not collide with other test * update outdated expect * Revert "only connect to socket if path has been replaced, fixes cypress-io#4776" This reverts commit f179eda. * update gulp in root * update https-proxy unit tests * update network spec to properly close server * update reporter spec * update https-proxy-agent to fix node 10.10.0 change discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799 * only pass --max-http-header-size on node >=12 * use own server-destroy implementation that supports secureConnect events * oops * update socket_spec * electron 6.0.0 * console.table introduced in node 10 * change browserify entry to init.js * handle edge case when no response body * console.table added in node 10 * do not exit app when all BrowserWindows are closed * update e2e snapshots * value may not be null * update plugins spec * correct cookie expiry, use browser.getversion for CDP version check * fix snapshotting for require stacks * reorder cookies in spec * warn when depreated electron callback apis are used * only report 1 plugin error per process * cleanup * node 12.4.0, cypress/browsers:node12.4.0-chrome76 docker image * update shell.openExternal to promisified * update dialog.showOpenDialog to promisified * update webContents.session.setProxy to promisified * updating native dependencies since we don't need ancient node ABI support anymore * WIP: switch cookies to simpler, jar-less approach * WIP: switch cookies to simpler, jar-less approach * making tests pass * improve cookie filtering logic * Remove unneeded Promise.try * filter what makes it to the extension * properly re-set superdomain cookies on cross-origin cy.visit * allow comma-separated list of e2e tests * sort cookies in order of expiration date, ascending * updating tests, cleanup * update tests * version electron as a devDependency, electron@6.0.1 * cleanup, remove old automation * cleanup, remove old automation * bump chokidar to fix win10 + node12 issue was seeing this on windows: nuxt/nuxt#6035 fixed with version bump * enable now-supported quit role, re-enable old tests * don't need that arg there * remove last deprecated callback electron invocations * Delete cypress.json * responding to PR feedback * cleanup * invoke * use 'quit' role * Use new appMenu role for Cypress menu on mac * electron@6.0.2 * electron@6.0.3 * remove domain: cookie.domain and see what happens * remove setErrorHandler * Revert "remove domain: cookie.domain and see what happens" This reverts commit 49e9168. * add unit tests for cookies * ci * fix project-content css * electron@6.0.4 * fix specs_list test * electron@6.0.7 * some cleanup * electron@6.0.9 * Update 8_reporters_spec.coffee.js * electron@5.0.10 - Chromium 73, Node 12 * cli: fix the STDIN pipe on Windows (cypress-io#5045) * cli: pipe stdin * uggh, here is the actual change * update cli unit tests * add unit test * more permissive check for json to include application/vnd.api+j… (cypress-io#5166) * more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON * fix eslint for fixture specs (cypress-io#5176) * update eslint to lint files within 'fixtures' in support files - ignore some edge cases like jquery, jsx and obvious js files we wrote with broken code * Fixes from eslint to 'fixtures' files * Catch env variable with reserved name CYPRESS_ENV 1621 (cypress-io#1626) * server: check CYPRESS_ENV variable when merging configs * catch invalid CYPRESS_ENV value in CLI, close cypress-io#1621 * linting * sanitize platform in test snapshot * linting * update error message text * add missing comma * fix finally merge in JS code * pass CLI linter * fix log reference, should be debug * use correct sinon reference * update message, show first part in red * update error message text * Addresses cypress-io#2953 (cypress-io#5174) * Addresses cypress-io#2953 * Added proper test for new error message * Didn't realize it ran this test as well, whoops * Implementing changes as suggested by @jennifer-shehane * Fixing tests and error output. Moved the checks to the start of the get command to ensure we always catch improper options * Removing issue test since the querying spec covers it * Using coffescript isArray check * depromisify things that were promisified b/t electron 5 <=> 6 Revert "update shell.openExternal to promisified" This reverts commit 8b6460d. Revert "update dialog.showOpenDialog to promisified" This reverts commit 5f178b0. Revert "update webContents.session.setProxy to promisified" This reverts commit 727df3a. * node12.4.0-chrome76 => node12.0.0-chrome75 * fix tests for electron downgrade * node12.0.0-chrome75 => node12.0.0-chrome73 Co-authored-by: Zach Bloomquist <github@chary.us> Co-authored-by: Brian Mann <brian.mann86@gmail.com>
Decided to go with the smallest possible change rather than introducing a new package (
type-is
) as suggested in the neighboring TODO.Closes #5165.
Pre-merge Tasks