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: move cy.type() event tests to its own file. #20191

Merged
merged 4 commits into from
Feb 24, 2022

Conversation

sainthkh
Copy link
Contributor

User facing changelog

N/A. It's just an internal cleanup.

Additional details

  • Why was this change necessary? => type_spec.js is too long. It takes about 2 minutes to finish. After adding event-related tests in this PR series, I thought it's a good idea to move them to its own file.
  • What is affected by this change? => N/A
  • Any implementation details to explain? => Simply moving tests.

How has the user experience changed?

N/A

PR Tasks

  • [na] Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?
  • [na] Have new configuration options been added to the cypress.schema.json?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Feb 15, 2022

Thanks for taking the time to open a PR!

Copy link
Contributor Author

@sainthkh sainthkh left a comment

Choose a reason for hiding this comment

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

There are a few minor changes to the tests to make them consistent.

Comment on lines +584 to +592
'#target-button-tag',
'#target-input-button',
'#target-input-image',
'#target-input-reset',
'#target-input-submit',
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 added #target to make tests consistent with cy.type(' ') tests.

it(target, () => {
cy.get(target).focus().type('{enter}')

cy.get('li').should('have.length', 4)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line has been added not to allow more than 4 lis.

it(target, () => {
cy.get(target).focus().type('{enter}')

cy.get('li').should('have.length', 3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line has been added not to allow more than 3 lis.

Comment on lines +606 to +611
'#target-input-checkbox',
'#target-input-radio',
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 added #target to make tests consistent with cy.type(' ') tests.

@sainthkh sainthkh marked this pull request as ready for review February 15, 2022 02:02
@sainthkh sainthkh requested a review from a team as a code owner February 15, 2022 02:02
@sainthkh sainthkh requested review from jennifer-shehane and removed request for a team February 15, 2022 02:02
Comment on lines +137 to +141
it('fires events in the correct order')

it('fires events for each key stroke')
Copy link
Member

Choose a reason for hiding this comment

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

lets remove these empty tests or log an issue to back-fill these.

Copy link
Contributor Author

@sainthkh sainthkh Feb 16, 2022

Choose a reason for hiding this comment

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

I tracked the history of type_spec.js and found out that it was like this long ago(d9bd8bf, link, it's a bit hard to track because type_spec became its own file from action_spec.).

IMHO, logging an issue and implementing these tests would be a better idea. But I'm not sure why it is left empty.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like a good idea. Mind logging the issue & add TODO with link to these?

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 opened this issue at #20283.

Comment on lines +352 to +362
// changed = 0

// cy.$$(":text:first").change ->
// changed += 1

// cy.get(":text:first").invoke("val", "foo").type("b{tab}").then ->
// expect(changed).to.eq 1
Copy link
Member

Choose a reason for hiding this comment

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

clean up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also like this long ago(d9bd8bf, link).

It looks like a valid test to me. IMHO, adding this to the issue above is a good idea.

cy.visit('fixtures/dom.html')
})

describe('events', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be appropriate to rename this to keyboard events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. keyboard events better describes the tests. I fixed it that way.

@BlueWinds BlueWinds merged commit 3d50292 into cypress-io:develop Feb 24, 2022
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.

3 participants