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

chore: make create function on server.ts obsolete #18615

Merged

Conversation

sainthkh
Copy link
Contributor

User facing changelog

N/A

Additional details

  • Why was this change necessary? => When defining types, I had to define dummy Server type to avoid type failures. This PR removes it.
  • What is affected by this change? => N/A
  • Any implementation details to explain? => I moved code inside create() to Server class.

How has the user experience changed?

N/A

Notes.

I also thought about removing create() classes in some other files, but it is not an easy convert because they gets state, cy, Cypress as their argument to generate functions.

Maybe, I need to find a workaround for them.

PR Tasks

  • Have tests been added/updated?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Oct 25, 2021

Thanks for taking the time to open a PR!

@sainthkh sainthkh mentioned this pull request Oct 25, 2021
4 tasks
@sainthkh sainthkh marked this pull request as ready for review October 26, 2021 01:33
@sainthkh sainthkh requested a review from a team as a code owner October 26, 2021 01:33
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team October 26, 2021 01:33
@jennifer-shehane jennifer-shehane removed their request for review October 27, 2021 14:28
Copy link
Contributor

@BlueWinds BlueWinds left a comment

Choose a reason for hiding this comment

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

The refactor itself looks good, only comments relate to the code under refactor.

packages/driver/src/cypress/server.ts Show resolved Hide resolved
// responseText may be undefined on some responseTypes
// https://github.com/cypress-io/cypress/issues/3008
// TODO: How do we want to handle other responseTypes?
this.responseTypeIsTextOrEmptyString(xhr.responseType) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a weird conditional, ending up as (xhr.responseType === '' || xhr.responseType === 'text') && xhr.responseType === '' - meaning responseTypeIsTextOrEmptyString is unnecessary here. And since this is the only place in the code it's used, could probably be removed entirely.

I know you're just moving around existing code here, but seems worth cleaning up a bit while moving around.

Copy link
Contributor Author

@sainthkh sainthkh Oct 28, 2021

Choose a reason for hiding this comment

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

I agree that it's really weird. But it seems that it is written in this way because of XMLHttpRequest's behavior: responseType is only accessible when its value is '' or 'text.

We should understand it like x && x.val when x is undefined.

Without this check, the test below will fail.

// https://github.com/cypress-io/cypress/issues/3008
it('aborts xhrs even when responseType not \'\' or \'text\'', () => {
let log = null
cy.on('log:changed', (attrs, l) => {
if (['xhr', 'request'].includes(attrs.name)) {
if (!log) {
log = l
}
}
})
cy
.window()
.then((win) => {
const xhr = new win.XMLHttpRequest()
xhr.responseType = 'arraybuffer'
xhr.open('GET', '/timeout?ms=1000')
xhr.send()
xhr.abort()
cy.wrap(null).should(() => {
expect(log.get('state')).to.eq('failed')
expect(xhr.aborted).to.be.true
})
})
})

packages/driver/src/cypress/server.ts Outdated Show resolved Hide resolved
BlueWinds
BlueWinds previously approved these changes Oct 28, 2021
const getStack = () => {
const err = new Error
export class Server {
options: any
Copy link
Member

Choose a reason for hiding this comment

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

Could we define these types to match the types defined in the CLI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These 2 are different. cy.server() is an old API that was used before cy.intercept() was created.

And this Server is Cypress.Server. It was also used internally for cy.server().

Only the public interface for Cypress.Server is defaults(). It seems that we have nothing to do for the types.

Copy link
Member

Choose a reason for hiding this comment

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

Gottchya. Makes sense! Thanks for the info.

// merge obj into defaults
return _.extend(serverDefaults, obj)
}
// The function below is used as a callback for lodash
Copy link
Member

Choose a reason for hiding this comment

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

This comment is a bit odd when reading through the code. Adding something like to cancel any pending xhrs would add more context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// add header properties for the xhr's id
// and the testId
this.setHeader(xhr, 'id', xhr.id)
// setHeader(xhr, "testId", options.testId)
Copy link
Member

Choose a reason for hiding this comment

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

This can probably be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tracked down the history and found out that these comments were introduced in 47410d5. It seems that it was a mistake.

emilyrohrbough
emilyrohrbough previously approved these changes Oct 28, 2021
@BlueWinds BlueWinds self-requested a review October 29, 2021 15:33
@emilyrohrbough emilyrohrbough merged commit 26e4f92 into cypress-io:develop Nov 1, 2021
tgriesser added a commit that referenced this pull request Nov 3, 2021
* develop: (40 commits)
  fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441)
  chore: make `create` function on server.ts obsolete (#18615)
  docs: Add instructions to squash commits to develop in Contributing (#18728)
  fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686)
  refactor: remove Ramda (#18723)
  chore: Increase paralleled machines for desktop-gui tests (#18725)
  chore: Update Chrome (stable) to 95.0.4638.69 (#18696)
  chore: release @cypress/vue-v3.0.4
  chore: release @cypress/react-v5.10.2
  chore: release @cypress/schematic-v1.5.3
  fix: remove outdated registry link (#18710)
  chore: release @cypress/schematic-v1.5.2
  chore: release create-cypress-tests-v1.1.3
  chore: Update Chrome (beta) to 96.0.4664.27 (#18676)
  chore(tests): Remove flaky assertion that relies on png how compression (#18668)
  fix: make sure to go back to no-specs when delete spec file (#17760)
  fix: Next.JS 12 components testing failing with ` TypeError: Cannot read property 'traceChild' of undefined` (#18648)
  Backport .gitignore from unified-desktop-gui
  chore(docs): add 'Upgrading Electron' instructions (#18594)
  release 8.7.0 [skip ci]
  ...
tgriesser added a commit that referenced this pull request Nov 3, 2021
* develop: (40 commits)
  fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441)
  chore: make `create` function on server.ts obsolete (#18615)
  docs: Add instructions to squash commits to develop in Contributing (#18728)
  fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686)
  refactor: remove Ramda (#18723)
  chore: Increase paralleled machines for desktop-gui tests (#18725)
  chore: Update Chrome (stable) to 95.0.4638.69 (#18696)
  chore: release @cypress/vue-v3.0.4
  chore: release @cypress/react-v5.10.2
  chore: release @cypress/schematic-v1.5.3
  fix: remove outdated registry link (#18710)
  chore: release @cypress/schematic-v1.5.2
  chore: release create-cypress-tests-v1.1.3
  chore: Update Chrome (beta) to 96.0.4664.27 (#18676)
  chore(tests): Remove flaky assertion that relies on png how compression (#18668)
  fix: make sure to go back to no-specs when delete spec file (#17760)
  fix: Next.JS 12 components testing failing with ` TypeError: Cannot read property 'traceChild' of undefined` (#18648)
  Backport .gitignore from unified-desktop-gui
  chore(docs): add 'Upgrading Electron' instructions (#18594)
  release 8.7.0 [skip ci]
  ...
tgriesser added a commit that referenced this pull request Nov 4, 2021
…ve-activeProject

* unified-desktop-gui: (57 commits)
  chore: Add e2e tests for global mode (#18719)
  chore: add percy to app and launchpad package (#18781)
  chore: update test
  refactor: move settings in app (#18729)
  feat: setup launchpad lifecycle (#18734)
  feat(app): decouple event manager from driver (#18695)
  chore: Force single resolution for core modules, infinite loop guard (#18764)
  fix(driver): Sticky elements within a fixed container will not prevent an element from being scrolled to (#18441)
  chore: cleaning up the runner container pattern (#18741)
  feat: Use .config files (#18578)
  chore(app): basic style and example to stop scrollIntoView bug (#18736)
  chore: make `create` function on server.ts obsolete (#18615)
  feat: add codegen utility (#18708)
  docs: Add instructions to squash commits to develop in Contributing (#18728)
  fix(@cypress/react): throw if using Next.js swc-loader without nodeVersion=system (#18686)
  refactor: remove Ramda (#18723)
  fix: support using create-cypress-tests as part of build process (#18714)
  chore: Increase paralleled machines for desktop-gui tests (#18725)
  fix(app): do not cache graphql (#18716)
  chore: Update Chrome (stable) to 95.0.4638.69 (#18696)
  ...
flotwig pushed a commit that referenced this pull request Nov 8, 2021
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
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.

None yet

3 participants