Add About Page E2E tests #162

dgodongm · 2023-09-15T18:10:41Z

Description

Main change is addition of aboutpage.cy.ts to add some basic About page tests. There are basic tests for each of the 3 major sections/components (About Header, About Banner, and About Details). The page doesn't really have functionality so this is more of a "render" test suite.
I also modified a couple of components to include data-cy attributes to more reliably identify key elements.

Related Issue

#129, #130, #158 - This PR provides E2E tests, rather than component tests, for these issues.

Motivation and Context

Provides feature coverage for About page and its sub-components. The E2E tests in this PR may suffice in place of component tests though perhaps if components get configurable options, then component tests would also be warranted. Open to feedback on which approach (E2E or component or both) would be preferred.

How Has This Been Tested?

Run in Cypress app using Chrome browser

vercel · 2023-09-15T18:10:44Z

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

@TechIsHiring first needs to authorize it.

dgodongm · 2023-09-15T18:14:23Z

@chadstewart @marktnoonan Could you take a look at this PR? One question (besides the E2E vs component test one mentioned in the description) is whether string checks are warranted (also any plans to move strings into resource files to enable localization?).

marktnoonan · 2023-09-18T14:18:48Z

Thanks @dgodongm, I'll check this out in the next few days!

dgodongm · 2023-09-27T16:32:07Z

@marktnoonan @chadstewart Gentle ping to see if you might have a chance to take a look this week.

marktnoonan · 2023-09-27T16:53:21Z

thanks @dgodongm and apologies for my slow response!

marktnoonan · 2023-10-01T15:39:26Z

@dgodongm thanks for adding these tests! I'm going to roll through the tickets and check if all expected tests exist:

#129

Tests if the About page banner renders properly
Tests if the 'Ask questions section' renders properly and the ~~button opens your email~~ (outdated requirement, current behavior navigate to Contact page)
Tests if the 'Find us on Social Media' component renders properly and links to the proper social media -- this is not tested yet, the viewport of the test needs to be increased to show this section of the component

#130

Test if the About Page content renders properly - since all this does is render 3 other components, it's covered since the tests cover the 3 other components

#158

an article element is used for the content
[-] Twitter, LinkedIn, and YouTube links exist (see cy.validateLink())
Anything else you can think of that is important, but we don't need to verify all of the text content since it's fine for text content to change without failing a test.

I'll make some comments inline the PR with suggestions of how we might make the tests a bit more specific.

I think there are good reasons to move some of this coverage to component tests, since I see e2e tests as higher level "journey" type tests, and component tests as more specific detail-oriented tests of the behavior, but in this situation there's no need to block on that or ask you to redo work - let's get this coverage merged in e2e and then we can move it around as needed.

marktnoonan

I left a few comments about possible refactors, and a couple about missing tests or changes. Please add the missing tests, but feel free to take or leave the suggested changes to contains. I see test IDs as an escape hatch to use sparingly, but it's not a widely held opinion.

As to component vs e2e - I definitely find it simpler to keep tests as close as possible to the code that is responsible for rendering parts of the page, it makes selectors a bit easier and the component tests run faster, plus when a developer works on the component they know exactly what specs to run. So ideally these would be component specs and the e2e spec might just be a little health check to make sure the page renders.

But definitely not going to block on that in this repo, e2e works just fine for this. No need for you to refactor this PR - but my recommendation is to only close the e2e issue, not the component test issue.

marktnoonan · 2023-10-01T15:43:53Z

cypress/e2e/aboutpage.cy.ts

+  });
+
+  it("About details renders", () => {
+    cy.get(':nth-child(3) > article').should("be.visible")


We can remove the nth-child(3) > from the locator here, the test shouldn't fail if the DOM nesting changes due to new code in the feature. Since you have some chained assertions within the article, it's probably safe to just get the article without any other qualifiers. If something happens to where more than one article element is on the page, the within command will yell at us and we can get more specific then.

marktnoonan · 2023-10-01T15:49:38Z

cypress/e2e/aboutpage.cy.ts

+        cy.validateLink('Chad R. Stewart', 'https://www.linkedin.com/in/ChadRStewart/');
+        cy.validateLink('#TechIsHiring', 'https://twitter.com/TechIsHiring/');
+        cy.validateLink('Hire Chad R. Stewart', '/hire-chad');


For these, the issue wasn't too clear as written, but I was thinking specifically of covering these links at the bottom in the "Follow Us" section:

I like that you asserted the links in the article content though, seems fine to leave those in, let's just add separate assertions for the "Follow Us" stuff.

added tests for "Follow Us" section

marktnoonan · 2023-10-01T15:53:41Z

cypress/e2e/aboutpage.cy.ts

+    cy.get('[data-cy="about-banner-header"]')
+      .should("be.visible")
+      .and("contain", "Have a question?");


How about:

cy.contains('h4', 'Have a question?') .should("be.visible")

Specifying the expected heading level is good for accessibility and by using the text of the heading we can be more specific about what is supposed to be happening, and avoid a test id.

@marktnoonan I like how your suggestion avoids a new test id, but am concerned about a check for h4, when it really shouldn't be an h4 from an a11y perspective. That is, headings should only increase by one level at a time and this skips from h1 to h4. A check for h4 suggests otherwise. I suppose I could implement your suggestion and log an issue about the skipped heading level (which would require an app fix and test fix). What do you think?

@dgodongm lol I didn't check the heading nesting my bad, I assumed it was suitable. Let's not specify the h4 here then, since we know it's wrong. But please do make a new issue if you have the time so we can correct the heading nesting (and maybe fix any style side effects?) and then we can update the test as well.

I also would be totally fine if you just fix the nesting in this PR so that this heading is at the correct level, if it doesn't look like it will cascade into a whole bunch of document structure fixes.

@marktnoonan I'll log an issue. I think it will be a bit more involved to fix the heading issues correctly so heading levels of re-usable components (molecules) can be set dynamically depending on context. There's also the issue of there being multiple h1 headers on the About page (from hard-coded h1 elements in molecules)

Logged #166 and #167 for the a11y issues

marktnoonan · 2023-10-01T15:57:02Z

cypress/e2e/aboutpage.cy.ts

+    cy.get('[data-cy="about-banner-body"]')
+      .should("be.visible")
+      .and("contain", "If you have any questions, please contact us");


Similarly, we can do

cy.contains('p', 'If you have any questions, please contact us') .should("be.visible")

marktnoonan · 2023-10-01T15:58:35Z

cypress/e2e/aboutpage.cy.ts

+    cy.get('[data-cy="about-banner-body"]')
+      .should("be.visible")
+      .and("contain", "If you have any questions, please contact us");
+    cy.get('[data-cy="about-banner-button"]')


This is a good use of data-cy - there's not a great way to use the validateLink helper to target a specific locator. It makes me want to allow that helper to take a new argument and allow scoping to a locator like you've done here.

marktnoonan · 2023-10-01T16:02:18Z

cypress/e2e/aboutpage.cy.ts

+      .within(() => {
+        cy.validateLink('Contact Us', '/contact');
+      });
+  });


What's missing here is a test of the content on the right hand side, which only appears on wider viewports:

I'd suggest we change the default viewport size for e2e in cypress.config.ts to 1400, like this:

e2e: { baseUrl: 'http://localhost:3000', viewportWidth: 1400, ...

That will make all tests of "desktop layout" be more representative.

Added tests for the rhs content

marktnoonan · 2023-10-01T16:05:01Z

cypress/e2e/aboutpage.cy.ts

+    cy.get('[data-cy="about-header-header"]')
+      .should("be.visible")
+      .and("contain", "Transnational Job Listing Channel");
+    cy.get('[data-cy="about-header-body"]')
+      .should("be.visible")
+      .and("contain", "So many jobs available, all you have to do is keep up with  our posts. Check below for recent job openings.");


We could refactor this to be a little shorter using contains like I suggest below, super optional.

dgodongm · 2023-10-04T17:51:56Z

src/components/molecules/about-header/about-header.tsx

                Transnational <span className="text-[#7AB8F1]">Job Listing</span> Channel
            </HeaderText>

            <div className="text-white flex justify-start items-start lg:px-5 w-full lg:w-[40%] text-left lg:text-center">
              <p className="">
-                So many jobs available, all you have to do is keep up with  our posts. Check below for recent job openings. 
+                So many jobs available, all you have to do is keep up with our posts. Check below for recent job openings. 


Removed extraneous whitespace as it was messing up the cy.contains check (oddly should('contains', ...) was fine with it)

dgodongm · 2023-10-04T17:53:13Z

@marktnoonan I made a pass at addressing your feedback. Let me know what you think. cc @chadstewart

dgodongm · 2023-10-10T13:30:42Z

@marktnoonan checking back on if the feedback revisions look ok

dgodongm · 2023-10-16T20:56:54Z

@marktnoonan Friendly nudge on this

Add About Page E2E tests

a55212c

marktnoonan requested changes Oct 1, 2023

View reviewed changes

address review feedback

76d0838

dgodongm commented Oct 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add About Page E2E tests #162

Add About Page E2E tests #162

dgodongm commented Sep 15, 2023

vercel bot commented Sep 15, 2023

dgodongm commented Sep 15, 2023

marktnoonan commented Sep 18, 2023

dgodongm commented Sep 27, 2023

marktnoonan commented Sep 27, 2023

marktnoonan commented Oct 1, 2023

marktnoonan left a comment

marktnoonan Oct 1, 2023

marktnoonan Oct 1, 2023

dgodongm Oct 4, 2023

marktnoonan Oct 1, 2023

dgodongm Oct 2, 2023

marktnoonan Oct 2, 2023

dgodongm Oct 2, 2023

dgodongm Oct 3, 2023

marktnoonan Oct 1, 2023

marktnoonan Oct 1, 2023

marktnoonan Oct 1, 2023

dgodongm Oct 4, 2023

marktnoonan Oct 1, 2023

dgodongm Oct 4, 2023

dgodongm commented Oct 4, 2023

dgodongm commented Oct 10, 2023

dgodongm commented Oct 16, 2023

Add About Page E2E tests #162

Are you sure you want to change the base?

Add About Page E2E tests #162

Conversation

dgodongm commented Sep 15, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

vercel bot commented Sep 15, 2023

dgodongm commented Sep 15, 2023

marktnoonan commented Sep 18, 2023

dgodongm commented Sep 27, 2023

marktnoonan commented Sep 27, 2023

marktnoonan commented Oct 1, 2023

marktnoonan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgodongm commented Oct 4, 2023

dgodongm commented Oct 10, 2023

dgodongm commented Oct 16, 2023