search

learn-co-curriculum / react-hooks-props-basics-lab

Other
6 stars 6.19k forks source link

Content Clarity #5

Closed bdenney closed 2 years ago

bdenney commented 2 years ago

Canvas Link

https://learning.flatironschool.com/courses/4542/assignments/150030?module_item_id=318021

Concern

Running through this lab, I spent a large amount of time trying to figure out why my test were failing despite the output appearing to be correct.

After digging through the tests I figured out that toBeInTheDocument() seems to only return the text content of the HTML tags and not the values of the various attributes. Upon closer inspection of the image on the lesson I do indeed see that it has the 'Links' URLs written out rather than the words 'GIthub' or 'LinkedIn', but I feel failing the tests on this is not really fair as the requirement only stated the links needed to be present.

If it's agreed that this path is a good one to go down, I'm happy to make the PR myself 😄

Additional Context

No response

Suggested Changes

I think a useful tweak would be to change the relevant tests to instead test for the href parameter. I'm definitely no React expert (yet!) but I tweaked the tests to produce what seemed to be what I was after like so:

test("passes 'github' to <Links> as a prop, via <About>", () => {
  const {container} = render(<App />);
  const a = container.querySelector(`a[href="${user.links.github}"`)
  expect(a).toBeInTheDocument();
  expect(a.tagName).toEqual("A");
});
ihollander commented 2 years ago

Hi @bdenney - thanks for your feedback! In this case, the tests are checking for the text that's displayed on the page for a couple reasons. One is accessibility: if a user of the application we're building in the lab is using a screenreader, they would use the text content to determine what content is on the page, and our tests are written with that in mind whenever possible.

Another is that the demo image and the deliverables both show that this feature should display the link as text, though this could be made clearer in the instructions:

I'll make some updates to this lesson to clarify. We'll also look into adding more information on how to read the tests in an earlier lesson so students are able to understand more clearly what the test is expecting in their components.

ihollander commented 2 years ago

Added material to an earlier lab to help explain test code: https://github.com/learn-co-curriculum/react-hooks-running-tests/pull/7

bdenney commented 2 years ago

screen.debug() is clutch! 👯 Thanks for the additional clarity 😄