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

Use chrome for testing #2726

Merged
merged 24 commits into from
May 24, 2024
Merged

Use chrome for testing #2726

merged 24 commits into from
May 24, 2024

Conversation

zabil
Copy link
Member

@zabil zabil commented May 14, 2024

Fixes: #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
Copy link
Member Author

zabil commented May 16, 2024

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. a9e87e5

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
Copy link
Member

Let me also check, if I can recollect something 👍

@zabil
Copy link
Member Author

zabil commented May 17, 2024

@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.

Apply security fixes

Use official repos for downloading

Migrate to use typescript

Store download urls in package json

Use new download url's for browser

Re-organize packages

Use chrome for testing

Update files to refer to the re-organized browser files.
Update chrome remote interface

Remove old taiko version info

Move platform resolution to metadata

Append 64 to linux

Fix downloadURL usage

Initialize metadata globally

Use the right platform

Add platform append

Initialize browser metadata outside constructor

Fix typescript versions issues for test

Use latest version of mocha

Uncomment the close browser test

Temporary commit to check behavior

Add prettier fomatting rules for windows

Add a mocha exit option

Upgrade docs test package

Commit the functional tests package-lock json file

Do clean install to remove local node modules

Deprecate beforeunload

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
zabil added 9 commits May 21, 2024 14:28
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Remove docs test on windows

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
@zabil zabil marked this pull request as ready for review May 22, 2024 14:29
@zabil
Copy link
Member Author

zabil commented May 23, 2024

@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
Copy link
Member

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
Copy link
Member Author

zabil commented May 24, 2024

Oh thanks! please push the changes if you can.

zabil and others added 11 commits May 24, 2024 14:11
Apply security fixes

Use official repos for downloading

Migrate to use typescript

Store download urls in package json

Use new download url's for browser

Re-organize packages

Use chrome for testing

Update files to refer to the re-organized browser files.
Update chrome remote interface

Remove old taiko version info

Move platform resolution to metadata

Append 64 to linux

Fix downloadURL usage

Initialize metadata globally

Use the right platform

Add platform append

Initialize browser metadata outside constructor

Fix typescript versions issues for test

Use latest version of mocha

Uncomment the close browser test

Temporary commit to check behavior

Add prettier fomatting rules for windows

Add a mocha exit option

Upgrade docs test package

Commit the functional tests package-lock json file

Do clean install to remove local node modules

Deprecate beforeunload

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Remove docs test on windows

Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
- fix the test by updating the script

Signed-off-by: Nivedha <nivedhasenthil@gmail.com>
Signed-off-by: Zabil Cheriya Maliackal <zabilcm@gmail.com>
@zabil
Copy link
Member Author

zabil commented May 24, 2024

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
Copy link

gaugebot bot commented May 24, 2024

@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
Copy link
Member

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
Copy link
Member Author

zabil commented May 24, 2024

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

@NivedhaSenthil NivedhaSenthil merged commit 296d4a6 into master May 24, 2024
9 of 11 checks passed
@zabil
Copy link
Member Author

zabil commented May 24, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Update to use Chrome for Testing
2 participants