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.

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

test: add first e2e test #1040

Merged
merged 19 commits into from
Aug 12, 2024
Merged

Conversation

shootermv
Copy link
Contributor

@shootermv shootermv commented Jul 25, 2024

Summary of changes

Checklist

  • I have read and followed the contribution guidelines.
  • I have read through the Code of Conduct and agree to abide by the rules.
  • This PR is for one of the available issues and is not a PR for an issue already assigned to someone else.
  • My PR title has a short descriptive name so the maintainers can get an idea of what the PR is about.
  • I have provided a summary of my changes.

closes #995

Copy link

socket-security bot commented Jul 25, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher

View full report↗︎

@shootermv shootermv force-pushed the add-first-e2e-test branch 7 times, most recently from 1143352 to 57b4650 Compare July 28, 2024 09:03
Copy link
Contributor

@jdwilkin4 jdwilkin4 left a comment

Choose a reason for hiding this comment

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

Tested it locally and the tests are working on my end.
Left a few comments 👍

Also, the README needs to be updated to mention the e2e tests and how to run them.

.github/workflows/playwright.yml Outdated Show resolved Hide resolved
playwright.config.ts Show resolved Hide resolved
@shootermv
Copy link
Contributor Author

shootermv commented Aug 2, 2024

will be very glad if some more people from main fcc project team can review this PR and give a feedbacks
@huyenltnguyen or @Sembauke
not sure i have learned all the best practices of playwright

Copy link
Member

@huyenltnguyen huyenltnguyen left a comment

Choose a reason for hiding this comment

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

Hey @shootermv, sorry for the delay in reviewing the PR.

I would like to clarify a bit. Are we going to build an API / server for the quiz data?

Right now I'm seeing our data being static and as constants: https://github.com/freeCodeCamp/Developer_Quiz_Site/tree/main/src/data, so I'm not sure what e2e tests are solving.

Usually we write e2e tests when our system involves multiple pieces (client + server + database + third-party libraries), and we want to test the app as a whole. The main repo needs e2e because it's too complex to test everything with just unit tests.

However, for Developer Quiz, everything happens on the client, so I think it is simple enough that unit tests are sufficient. Also, unit tests are much faster than e2e tests.

@jdwilkin4
Copy link
Contributor

@huyenltnguyen

Are we going to build an API / server for the quiz data?

No.

That is not a priority for this project. Nor is it needed for this type of project.
This project will always be smaller scale with everything on the client.

For extra context, this project mainly serves as a beginner friendly way to have people get started with contributing and it is less intimidating than the main repo obviously 😄

However, for Developer Quiz, everything happens on the client, so I think it is simple enough that unit tests are sufficient. Also, unit tests are much faster than e2e tests.

Yeah, I can see your point here.

My main reason for green lighting this addition is that, it is not really going to hurt adding this.
I think just have a few tests and calling it a day will be sufficient for this type of project

@a2937
Copy link
Member

a2937 commented Aug 12, 2024

While unit tests are sufficient in ensuring the data and some behavior is fine, it does lack the ability to check the website's CSS and anything related to having an actual view of the website.

Personally I think adding tests for this kind of thing is wonderful , at least to have the ability to test the blind spots.

Copy link
Member

@huyenltnguyen huyenltnguyen left a comment

Choose a reason for hiding this comment

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

@jdwilkin4 Thank you for the clarification. And thank you, @shootermv, for the PR (and sorry again for not reviewing this in a more timely manner).

To speed up the review process, I went ahead and made changes to the PR. The changes were split to small commits so you can see exactly what I did.

In summary, I:

  • Changed the workflow to only target the main branch (I don't think we will ever have a master branch)
  • Specified the exact version of Node that the workflow should run on (the majority of our repos are on v20)
  • Renamed sanity.spec.ts to landing-page.spec.ts
    • I found that the display logic of the /quizzes page was duplicate (in sanity and can-run-quiz), so I removed the test from sanity, making the file purely for the landing page (hence the rename).
  • Removed unnecessary comments from the tests (the code itself is already self-descriptive)
  • Replaced queries that involve implementation details with queries by name
    • Some examples are: querying buttons by their order in the DOM, querying modal content by their class name or ID. These are implementation details, and they can make the tests brittle as we could change class names or IDs in the future while keeping the functionality intact.

I kept the tests as-is, but I feel obligated to say this as someone who has been working with a lot of E2E tests in the main repo as well as someone who is interested in testing approaches in general.

  • For tests that only test the page display without any user action (those that navigates to a page and check if certain elements exist), I think they should be written with unit tests, I'd even write unit tests for some small user interactions, using the user-event package from React Testing Library.
  • For tests that simulate the entire flow (go to a page, click button X, click button Y, a modal shows up, click button Z, see something else, etc.), they should be written with E2E tests.

The practical reason is, this design is better for performance / completion time. E2E tests are expensive to run, so it's wasteful if we have to go through the setup and teardown process just to check if a line of text exists on the page (we had a lot of mini cases like this in the main repo). And the ones in can-run-quiz.spec.ts are those that I personally would convert to unit tests.

That's my two cents on how I'd approach testing this app if I need to involve E2E. Keeping things as-is isn't an issue, as long as we don't default to E2E for everything 🙂

@huyenltnguyen huyenltnguyen merged commit c0c5839 into freeCodeCamp:main Aug 12, 2024
3 checks passed
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.

[Testing] - add e2e tests using playwright
4 participants