Skip to content

Refactor out header checks in homepage E2E tests#165

Open
dgodongm wants to merge 2 commits intoTechIsHiring:devfrom
dgodongm:dg_refactor_header_checks
Open

Refactor out header checks in homepage E2E tests#165
dgodongm wants to merge 2 commits intoTechIsHiring:devfrom
dgodongm:dg_refactor_header_checks

Conversation

@dgodongm
Copy link
Contributor

Description

Created validateHeader util module with helper methods similar to validateFooter, and updated homepage E2E tests to use these, adding distinct test for mobile as well.

Related Issue

#121 - provides some E2E coverage instead of component as it seems there's technical impediment for the latter - see comments in this issue

Motivation and Context

Ensure more consistent approach between footer and header checks. And, adding some coverage for modal menu on mobile.

How Has This Been Tested?

Tested locally on Chrome

Screenshots (if appropriate):

n/a

@vercel
Copy link

vercel bot commented Sep 27, 2023

Someone is attempting to deploy a commit to a Personal Account owned by @TechIsHiring on Vercel.

@TechIsHiring first needs to authorize it.

@dgodongm
Copy link
Contributor Author

@chadstewart @marktnoonan What do you think of this refactoring, and additional mobile test?

Copy link
Collaborator

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

This looks great, nice to capture mobile and since everything is the same it makes perfect sense to pull the repeated code into a helper function.

Also there's a good point raised about component testing and the latest version of next, let's not sink too much time into solving those issues right away if the same tests will work fine from e2e.

@dgodongm
Copy link
Contributor Author

dgodongm commented Oct 2, 2023

Thanks @marktnoonan! @chadstewart - any feedback or are you good with merging?

@dgodongm
Copy link
Contributor Author

@chadstewart @marktnoonan Can this be merged?

@marktnoonan
Copy link
Collaborator

@chadstewart this needs you to authorize the Vercel step. Or I can merge without that since it's just tests.

@dgodongm
Copy link
Contributor Author

@chadstewart Friendly ping on this PR

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.

2 participants