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

docs: test commands #91

Merged
merged 3 commits into from
Oct 5, 2023
Merged

docs: test commands #91

merged 3 commits into from
Oct 5, 2023

Conversation

NoamGaash
Copy link
Member

explain about out test suite

@@ -23,6 +23,11 @@ This app is created by the volunteers of [Public Knowledge Workshop](https://www
- `yarn install`
- `yarn start`

### testing the project:
- running the tests - `yarn playwright test`
Copy link
Collaborator

Choose a reason for hiding this comment

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

image

we have script for it
npm run test:playwright

In addition, What do you think about changing the scripts?
"scripts": { ... "test": "playwright test", "test:ui": "playwright test --ui" ... }

Do we need the "react-scripts test" when we use playwright?
if so we should add,
"test:react-scripts": "react-scripts test"
because we don't use it so often

Afterwards, we will change the docs accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding the scripts, I like your suggestion.
Regarding npm run test:playwright - we use yarn, and I'm not sure about the best practice. maybe yarn run test:playwright.
we don't need react-scripts test, but that's for another PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you right , my mistake. I meant yarn run test:playwright in the current scripts.
If you would like first to change the scripts and then change the documentation, the commands will be yarn run test / yarn run test:ui
Tell me what you prefer.

Copy link
Member Author

Choose a reason for hiding this comment

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

having yarn run test and yarn run test:ui sounds great

@NoamGaash NoamGaash merged commit d9139c5 into main Oct 5, 2023
4 checks passed
@NoamGaash NoamGaash deleted the docs/tests branch October 5, 2023 07:06
@ArkadiK94
Copy link
Collaborator

Maybe there was a misunderstanding. Do you want me to open an issue about changing the scripts and the Read.me afterwards? (I thought you will make these changes before merging)

@NoamGaash
Copy link
Member Author

I thought you will open a PR, but I can do it as well

@ArkadiK94
Copy link
Collaborator

Ok, my mistake, I will do it, thanks anyway :)

@NoamGaash
Copy link
Member Author

thank you! #97 great job 💪

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.

2 participants