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

Feature/add cypress #3062

Closed
wants to merge 7 commits into from
Closed

Conversation

nash1111
Copy link

Description

I have created 3 e2e tests for todomvc using cypress. And i added to the README how to run the tests.

  1. Todo can be created successfully
  2. Links are successfully attached
  3. The specified port is accessible
    I would be glad if someone could review it. Thanks in advance.

Checklist

  • I have reviewed my own code
  • I have added tests

@github-actions
Copy link

github-actions bot commented Dec 25, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 107.144 107.145 +0.001 +0.001%
boids 171.188 171.187 -0.001 -0.001%
communication_child_to_parent 91.266 91.267 +0.001 +0.001%
communication_grandchild_with_grandparent 105.618 105.620 +0.002 +0.002%
communication_grandparent_to_grandchild 101.460 101.463 +0.003 +0.003%
communication_parent_to_child 88.356 88.356 0 0.000%
contexts 108.137 108.140 +0.003 +0.003%
counter 86.296 86.295 -0.001 -0.001%
counter_functional 86.605 86.607 +0.002 +0.002%
dyn_create_destroy_apps 89.120 89.117 -0.003 -0.003%
file_upload 100.767 100.766 -0.001 -0.001%
function_memory_game 165.280 165.267 -0.014 -0.008%
function_router 349.839 349.824 -0.015 -0.004%
function_todomvc 160.229 160.224 -0.006 -0.004%
futures 225.326 225.325 -0.001 -0.000%
game_of_life 107.035 107.035 0 0.000%
immutable 182.006 182.000 -0.006 -0.003%
inner_html 82.713 82.713 0 0.000%
js_callback 112.280 112.291 +0.011 +0.010%
keyed_list 196.719 196.721 +0.002 +0.001%
mount_point 85.864 85.865 +0.001 +0.001%
nested_list 113.861 113.862 +0.001 +0.001%
node_refs 93.771 93.765 -0.007 -0.007%
password_strength 1550.222 1550.225 +0.003 +0.000%
portals 97.057 97.055 -0.002 -0.002%
router 319.918 319.918 0 0.000%
simple_ssr 151.865 151.866 +0.001 +0.001%
ssr_router 394.287 394.282 -0.005 -0.001%
suspense 109.420 109.431 +0.011 +0.010%
timer 89.163 89.165 +0.002 +0.002%
todomvc 141.659 141.682 +0.022 +0.016%
two_apps 86.915 86.917 +0.002 +0.002%
web_worker_fib 152.193 152.175 -0.019 -0.012%
webgl 85.422 85.420 -0.002 -0.002%

✅ None of the examples has changed their size significantly.

@ranile
Copy link
Member

ranile commented Dec 25, 2022

My 2 cents: that's a lot of JS code that I'm not really comfortable with putting in examples. It would also need to run in CI. None of it is Yew specific and I think we're better served by mentioning it in our documentation and redirecting users to cypress' docs

@WorldSEnder
Copy link
Member

WorldSEnder commented Dec 26, 2022

I actually like the idea. Weighing between not having end-to-end tests and having them based on javascript, I prefer the latter - given that they do tend to break from time to time partly because we're using the nightly compiler for the deployment.

But: Do you have a blogpost or short explanation on how to use this? I'm not sure how best to run the tests locally, the README you're refering to doesn't seem to be part of the PR.

Where I agree with hamza is that this e2e setup should perhaps not be a part of a specific example, but form a test-suite we run as part of CI.

@nash1111
Copy link
Author

I forgot to push the README so I pushed it, sorry. I think with the instructions I wrote in the README you can start cypress e2e test following it

I agree with both of you, doing manual e2e testing only for certain EXAMPLE is a bad idea and should be incorporated into CI

So I have a question, is there a standard project that should be e2e tested? Or should we create one?

@WorldSEnder
Copy link
Member

WorldSEnder commented Dec 26, 2022

So I have a question, is there a standard project that should be e2e tested? Or should we create one?

There is no "one project" currently, the examples are built and then pasted together for deployment, see build-examples.sh which is used in the workflow that deploys them. It would make sense to me to have e2e testing somewhere in the tools/ directory.

@ranile
Copy link
Member

ranile commented Dec 26, 2022

If we're using JS for end-to-end testing, I would prefer it to based on Playwright. Cypress is great but for this particular use-case, it seems too bulky (it has so much functionality like API routes mocking, fixtures and such) that we don't need, since the examples don't really rely on user input. It also supports screenshot testing out of the box, which Cypress does not.

We can have examples/<example_name>/specs/*.spec.ts files that Playwright will use. The config will file will be placed at examples/playwright.config.ts. I should be able PR the configuration set-up soon.

I appreciate your effort for making this PR and getting a discussion started. It seems most of the code in the PR is from the template (I don't want your effort to be for nothing). It would be great if we moved this discussion to an issue (there tends to be more activity). See #196, or if you want, you can create a new issue

@futursolo
Copy link
Member

I am indifferent between Cypress and Playwright if the notion is to adopt a JavaScript-based Testing Library for E2E tests.

Cypress provides everything out of the box so we do not need to worry anything about setting up the toolchain. I am usually against framework, but in this case this works in our favour as we do not want to devote too much time at setting up and maintaining JavaScript testing framework (jest?), bundler, etc for JavaScript testing.

However, I found Playwright to have be better at writing tests that works reliably and Cypress are more likely to be flaky and tend to break things upon major version upgrade.

Regardless of whether Cypress or Playwright are selected, I would like to see something like Yarn Workspace to be used so we do not end up with many separate and duplicated node_modules. Both Playwright and Cypress distribute their main binary outside of node_modules, but they still produce many dependencies in node_modules.

@ranile
Copy link
Member

ranile commented Dec 26, 2022

The thing about Playwright is that we can the test files where we want to. There can be only one package.json in the examples directory so duplication of node_modules can be avoided

"cypress:run": "cypress run --browser chrome"
},
"author": "",
"license": "ISC",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"license": "ISC",
"license": "(MIT OR Apache-2.0)",

Yew is licensed in MIT OR Apache-2.0 at the users choice, I believe tests should also follow this license.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review, I fixed it to MIT OR Apache-2.0

export default defineConfig({
e2e: {
baseUrl: "http://localhost:8080",
"defaultCommandTimeout": 30000
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the default command timeout to remain 4_000 as doing so may significantly extend test runtime if many tests start to fail (which is not uncommon if we are trying to update Yew itself.)

Copy link
Author

Choose a reason for hiding this comment

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

I changed it to 4_000

@futursolo
Copy link
Member

The thing about Playwright is that we can the test files where we want to. There can be only one package.json in the examples directory so duplication of node_modules can be avoided

You can also do it with by specifying specPattern in cypress.config.ts.

I just felt it might be better to give each example their own package.json because users can do cd examples/todomvc; yarn start & yarn test and not cd examples/todomvc; trunk serve & yarn test tests/*.spec.ts.

My 2 cents: that's a lot of JS code that I'm not really comfortable with putting in examples.

I forgot to mention,
I also feel there will be a lot of JavaScript code to be introduced into the repository if we make this precedence.
So I actually prefer sticking with wasm-pack unless there is a need for tests that cannot be authored with it.

@nash1111
Copy link
Author

nash1111 commented Dec 27, 2022

I greatly appreciate the productive responses from all of you!
I also think it is better to have e2e testing since we are using nightly build for deployment.
I think it is inevitable that some TS/JS code will get into the repository because of this.
In that case, of course, it is essential to separate dependencies using yarn workspace etc.

The reason Cypress looks bulky is due to my poor configuration, it is possible to cut it down and also take a screenshot

I too find it hard to decide between Cypress or playwright. I tend to prefer Cypress in terms of the large number of users and well maintained documentation, but either is fine with me.

I have two questions.
May I start writing tests for examples/?
When I do, I think it is better to follow this test design.
If so, how do I decide whether to use Cypress or playwright?

@nash1111
Copy link
Author

I agree with the policy that what can be tested with wasm-pack should be tested with wasm-pack.
However, I think it would be very difficult to run screenshots, screen transition recordings, etc. like cypress and playwright in wasm-pack, because it would mean re-implementing cypress and playwright functionality in wasm.
Hence I think it is better to use cypress or playwright for e2e testing and wasm-pack for rendering, type checking, etc.

@ranile
Copy link
Member

ranile commented Dec 29, 2022

I would prefer Playwright since the tests can be written in Rust. https://github.com/octaltree/playwright-rust by @octaltree exists but it seems it isn't maintained anymore: octaltree/playwright-rust#30

@Security Union#5828 on discord mentioned interest in making a playwright library. It was discussed in this thread on Yew discord but it hasn't seen any activity.

@nash1111
Copy link
Author

nash1111 commented Jan 4, 2023

Sorry for the late reply and thanks for the introduction to a good repository.
I will try to write a test with playwright-rust in another PR and will come back to this one if it is not possible or if it lacks features.

@voidpumpkin
Copy link
Member

I have been having great experience with rust library thirtyfour.
Perhaps that is also a direction we could consider taking.

Still it is not fully comparable as it only allows to use webdriver instead of being a full blown e2e testing framework like playwright or cypress.

@voidpumpkin voidpumpkin added the S-waiting-on-author Status: awaiting action from the author of the issue/PR label Apr 3, 2023
@nash1111
Copy link
Author

nash1111 commented Apr 4, 2023

Thank you all for your comments!
I'm closing this PR as I'm changing to using playwrightn for the time being.

@nash1111 nash1111 closed this Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: awaiting action from the author of the issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants