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.

Sign up for GitHub

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

Jump to bottom

Migrate old Cypress tests to use Testing Library features #378

Merged
merged 29 commits into from
Oct 20, 2021
Merged

Migrate old Cypress tests to use Testing Library features #378

merged 29 commits into from
Oct 20, 2021

Conversation

HonkingGoose
Copy link
Collaborator

@HonkingGoose HonkingGoose commented Sep 13, 2021 •
edited
Loading

Changes

  • Migrate old Cypress test so that I'm using Testing Library features more often

Context

Closes issue #373.

Marking this as a draft PR, as I need feedback and help! 😉

Todo's:

  • Get review by @Belco90
  • Implement review feedback/tips and tricks
  • Remove comments that are no longer needed
  • Final review and merge

HonkingGoose
HonkingGoose commented Sep 13, 2021
View reviewed changes
Copy link
Collaborator Author

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Some things I need help with! 😉

cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/comparator.spec.ts Show resolved Hide resolved
cypress/integration/home.spec.ts Outdated Show resolved Hide resolved
@HonkingGoose HonkingGoose mentioned this pull request Sep 13, 2021
Closed
6 tasks
Belco90
Belco90 requested changes Sep 13, 2021
View reviewed changes
cypress/integration/comparator.spec.ts Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/home.spec.ts Outdated Show resolved Hide resolved
cypress/integration/home.spec.ts Outdated Show resolved Hide resolved
cypress/integration/home.spec.ts Outdated Show resolved Hide resolved
HonkingGoose
HonkingGoose commented Sep 14, 2021
View reviewed changes
cypress/integration/comparator.spec.ts Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/home.spec.ts Outdated Show resolved Hide resolved
@Belco90 Belco90 marked this pull request as ready for review September 16, 2021 13:53
@Belco90 Belco90 marked this pull request as draft September 16, 2021 13:54
Belco90
Belco90 reviewed Sep 16, 2021
View reviewed changes
cypress/support/index.ts Outdated Show resolved Hide resolved
@Belco90
set empty string when env var is not defined
787b761 
This way we don't block tests to those running them without access token for testing.
@HonkingGoose
Copy link
Collaborator Author

Ready for a review on the work in progress, this should be a lot better than what we had at first, and it properly passes the ESLint tests now. 😄

Belco90
Belco90 requested changes Oct 19, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Looking good in general! Just a couple of tweaks, adding some extra assertions to the main comparator test and I think we are good to go.

cypress/integration/comparator.spec.ts Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
cypress/integration/comparator.spec.ts Show resolved Hide resolved
cypress/integration/home.spec.ts Show resolved Hide resolved
@HonkingGoose HonkingGoose mentioned this pull request Oct 19, 2021
Closed
@HonkingGoose HonkingGoose marked this pull request as ready for review October 19, 2021 16:47
HonkingGoose
HonkingGoose commented Oct 19, 2021
View reviewed changes
Copy link
Collaborator Author

@HonkingGoose HonkingGoose left a comment

Choose a reason for hiding this comment

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

Some things that we could change maybe?

cypress/integration/comparator.spec.ts Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@Belco90 @HonkingGoose
Apply suggestions from code review
3c8d04a 
Co-authored-by: HonkingGoose <34918129+HonkingGoose@users.noreply.github.com>
Belco90
Belco90 previously approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

LGTM!

@HonkingGoose HonkingGoose enabled auto-merge (squash) October 20, 2021 07:33
@HonkingGoose HonkingGoose merged commit 42f191c into main Oct 20, 2021
@HonkingGoose HonkingGoose deleted the honkinggoose/migrate-old-cypress-373 branch October 20, 2021 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Belco90 Belco90 Belco90 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Webapp - Site Redesign
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HonkingGoose @Belco90