getgauge / taiko

A node.js library for testing modern web applications
https://taiko.dev
MIT License
3.56k stars 452 forks source link

Use chrome for testing #2726

Closed zabil closed 3 months ago

zabil commented 3 months ago

Fixes: https://github.com/getgauge/taiko/issues/2708

Notes:

Deprecated beforeunload method because of the following issue https://developer.chrome.com/docs/web-platform/deprecating-unload

Also gauge-ts does not work on node versions greater than 16 on windows because of this issue https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2

Our functional tests uses gauge-ts and spawn is used in the launcher

Unfortunately to fix this we'll need big changes to gauge-ts

zabil commented 3 months ago

Hey @NivedhaSenthil . So I was able to make the changes to download chrome for testing and get the functional and unit tests to pass locally after making this commit. https://github.com/getgauge/taiko/pull/2726/commits/a9e87e53da958ac2b92ce4f2859d21eca07ed6d4

Which means there is an issue with register and hanging on to promisesToBeResolvedBeforeCloseBrowser.push. If this piece of code is there it fails to close the browser and the other tests get an issue while opening it.

I am taking a look at it but any pointers will be helpful!

NivedhaSenthil commented 3 months ago

Let me also check, if I can recollect something 👍

zabil commented 3 months ago

@NivedhaSenthil does this have anything to do with https://developer.chrome.com/docs/web-platform/deprecating-unload?

I noticed that in the tests the beforeunload event does not fire.

zabil commented 3 months ago

@saikrishna321 @NivedhaSenthil can you take a look at this PR. The build runs consistently on linux (and mac) which is actually an improvement over previous versions.

On windows (check PR description) there are a lot of issues especially with using gauge-ts on versions greater than 16. While the unit tests run correct locally do run they are unpredictable in the CI for windows same is the case with functional tests.

I would still recommend bumping up to version 1.4 so that there's a fallback to 1.3.

Tell me what you think

NivedhaSenthil commented 3 months ago

Hi @zabil, sorry for coming back late on this. I think beforeunload event is still not deprecated, The [beforeunload](https://developer.mozilla.org/docs/Web/API/Window/beforeunload_event) event has a slightly different use case to unload in that it is a cancellable event. It is often used to warn users of unsaved changes when navigating away. This event is also unrealiable as it will not fire if a background tab is killed. It is recommended to limit use of beforeunload and [only add it conditionally](https://developer.chrome.com/blog/page-lifecycle-api#the-beforeunload-event). Instead, use the above events for most unload replacements.

From https://developer.chrome.com/docs/web-platform/deprecating-unload. And guess it may be useful to have the api as there may be some sites still using it. And it looks like the event was not triggered for a different reason as mentioned in this blog.

Updating the html script in the beforeunload.test.js to below made the tests run fine.

    <div class="main">
        <label>Write your name : <input id='name' type="text" autofocus /> </label>
    </div>
    <script>
        let oldValue = document.getElementById('name').value;
        window.addEventListener('beforeunload', function (e) {
          let newValue = document.getElementById('name').value;
          if( oldValue != newValue) {
            event.preventDefault();
            // Legacy support for older browsers.
            return (event.returnValue = true);
          }
        });
    </script>

I have the changes locally, can push it if this looks fine.

zabil commented 3 months ago

Oh thanks! please push the changes if you can.

zabil commented 3 months ago

Sorry I am not sure what happened but the commits before unload changes seem to have disappeared after i did a push bumping up the version even though I didn't do a force push. Do you still have the commits?

gaugebot[bot] commented 3 months ago

@zabil Thank you for contributing to taiko. Your pull request has been labeled as a release candidate 🎉🎉.

Merging this PR will trigger a release.

Please bump up the version as part of this PR.

Instructions to bump the version can found at CONTRIBUTING.md

If the CONTRIBUTING.md file does not exist or does not include instructions about bumping up the version, please looks previous commits in git history to see what changes need to be done.

NivedhaSenthil commented 3 months ago

I managed to add the changes back but forgot to sign DCO :/ Trying to rebase is leading to conflicts, should we leave it as such ?

zabil commented 3 months ago

Yeah. I think it's fine if there are issues. Please feel free to merge if you are ok with the changes.

zabil commented 3 months ago

I'll take a look at the release pipeline https://github.com/getgauge/taiko/actions/runs/9222743853