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

#74 Installed Playwright and add in to CI/CD pipeline #92

Merged
merged 29 commits into from
Apr 11, 2024
Merged

Conversation

vmaineng
Copy link
Contributor

@vmaineng vmaineng commented Mar 25, 2024

  1. Installed Playwright (pnpm)
  2. Added in Playwright to ci.yaml file
  3. Added to Jest testing to ignore Playwright testing in this specific file
  4. Tested playwright example tests ran correctly

Copy link

vercel bot commented Mar 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gridiron-survivor ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2024 10:22pm

@vmaineng vmaineng changed the base branch from main to develop March 25, 2024 00:07
Copy link
Collaborator

@shashilo shashilo left a comment

Choose a reason for hiding this comment

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

@vmaineng comments for you to review.

use: { ...devices['Desktop Safari'] },
},

/* Test against mobile viewports. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove unused code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Shashi! Would you please elaborate as to why Desktop Safari should be removed? I believe it's not as popular but I can still see Safari being used to this day. Please let me know. :) Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I figured you removed it because they are commented out. If it's not meant to be commented out, bring it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the commented code.

* See https://playwright.dev/docs/test-configuration.
*/
export default defineConfig({
testDir: './tests',
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do you envision the structure of our test files? How will the file directory look like and where will the test files live? For example, if I have a component named Test.tsx, where will it's test file live in the folder structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Shashi! After doing some research,

  • for unit testing:
    the test files will have same co-location as the component file it's being tested on

  • for integration and E2E testing:
    It appears there is a top level directory folder named "playwright" that will consists of the tests.

For this line of code, I'd recommend changing the name folder from "tests" to "playwright" to show that it is E2E testing.

Please let me know what your thoughts are. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest keeping the test files together all in one directory. Giving them different file naming patterns will help determine which file is which. Also, I don't think this path will work for individual tests. I believe this is pointing to a root directory instead of the component directory themselves. Please test this out. Ensure that it will work inside directories and outside. Meaning, if it's named hello.spec.ts and lives inside /components/hello/hello.spec.ts, it should execute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashilo I modified testDir: './tests', to testDir: './', and tested tests would be able to run from any directories they reside in. Please let me know if you have any other observations. Thanks!

Copy link
Collaborator

@shashilo shashilo left a comment

Choose a reason for hiding this comment

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

@vmaineng please review my comments.

.gitignore Outdated
#playwright
/test-results/
/playwright-report/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this something we want to ignore? We will eventually create a deployment that can output the Playwright report so the person creating the PR can see this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashilo Thanks for explaining, Shashi. I will remove from the .gitignore file.

README.md Outdated

## React Testing Library (RTL) && Jest

Install dependencies
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to remove the installation from our dependencies. 1 installation is all a new user would need to run. Then the specific commands would be needed for the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shashilo Thanks for clarifying. I removed the pnpm i since only 1 installation is needed and left specific commands on how to run tests.

@shashilo shashilo enabled auto-merge (squash) April 9, 2024 20:49
Copy link
Contributor

@vazquezea96 vazquezea96 left a comment

Choose a reason for hiding this comment

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

Looks good!

@shashilo shashilo merged commit 9551865 into develop Apr 11, 2024
5 checks passed
@shashilo shashilo deleted the playwright branch April 11, 2024 15:23
Clue355 pushed a commit that referenced this pull request May 1, 2024
* installed playwright and added into the CICD pipeline

* added in Playwright and RTL in ReadMe

* cleaned up package.json

* deleted commented code that was not used and updated file directory for tests

* deleted test examples directories and modified test files

* removed test result and playwright report from .gitignore

* installed newest dependencies

* added in action-setupv2 in ci.yml

* removed the line I added

* revert pnpm install back to pnpm ci

* modified ci.yml with cache

* revert changes back

* commented out mobile views

* indented playwright testing

* modified indentation of yaml file

* revert back identation

* revert changes

* added in different install

* removed test files

* removed test folders

* added in example test file

* removed test example

* added in path to ignore playwright tests

* removed test results and playwright report out of .gitignore
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.

3 participants