Adding Home Page Unit Test #1858
Conversation
MalikMAlna
requested review from
fozziethebeat,
AbdBarho and
notmd
as code owners
| render(<Home />); | ||
| expect(screen.getByRole("heading", { level: 1 })).toHaveTextContent("Open Assistant"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Thank you, quick question, how would you specify the language here?
There was a problem hiding this comment.
I’m sorry, I’m not quite sure I understand your question?
Are you referring to the language of the code or what the code is doing? It’s just some JavaScript/TypeScript that checks if the Hone component is properly rendering on the homepage based on the h1 of Open Assistant showing up on the page using Jest. It’s a bit bare bones as far as unit testing goes, but I consider it a decent start to a good bit of unit test code coverage for the data collection website.
I hope that answers your question, but if not please let me know and I’ll be happy to respond.
There was a problem hiding this comment.
this test is checking for the text Open Assistant which is in the english language, however, this text will be different if the website is rendered in another language, for example, korean would be 오픈 어시스턴트
Our default is english, but it would be great to specify the language when doing the tests, to guarantee reproducibility.
There was a problem hiding this comment.
This is a unit test, language probably will fall back to English. But I'm not sure we want to test the page with unit test, e2e is more suitable for this thing.
There was a problem hiding this comment.
There is already a unit test for the about page that essentially does the same thing with English. So I’m not sure why that’s fine, but you all are hesitant about a unit test for the main page does a similar thing and is arguably more important than a unit test on whether the about page works.
There was a problem hiding this comment.
That being said, @AbdBarho I would be happy to “specify the language,” but I’m not exactly sure what you mean about that in the context of the unit test.
Could you please clarify?
There was a problem hiding this comment.
Furthermore, if the website will likely default back to English, I don’t honestly see the point in localizing the test for different languages because whatever is causing the homepage to not show up in English would likely be a problem with the homepage in other languages, right?
There are also languages that are not English where the component still says Open Assistant in the h1 on the homepage. So I do think the test largely covers for concerns on localization outside of some extremely specific localization edge cases.
There was a problem hiding this comment.
Actually, the more I look at the other localizations of the main page, it only looks like the Korean translation removes the english phrase “Open Assistant” from the h1 header with its actual translation. Most of the other translations do not. You guys may want to compare that with the consistency of the localizations for other languages.
There was a problem hiding this comment.
Also, @notmd I think e2e tests are more appropriate if we want to know that features on the main page work, but if the page component doesn’t even render in the first place then there’s nothing to test on the e2e to confirm whether that’s the case aside from all the tests just failing for what seems like no reason.
At least my unit test would let us know in a computationally inexpensive fashion, “oh, the page component isn’t rendering. That’s kind of weird” if any new changes broken the original functionality of that page component.
Thank you thank you. :) |
Noticed there wasn't a jest test for the home/index page. Decided to add one in case something caused that to not work and came up as a breaking change.
Also, I'm planning to add some more website unit tests over the next week or two. It wouldn't hurt for us to have those.