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

Add service worker to make multiple requests for the test page (closes #5239) #5738

Merged
merged 21 commits into from Dec 16, 2020

Conversation

AlexKamaev
Copy link
Contributor

No description provided.

@testcafe-build-bot
Copy link
Collaborator

1 similar comment
@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@AlexKamaev AlexKamaev closed this Nov 29, 2020
@AlexKamaev AlexKamaev reopened this Nov 29, 2020
@AlexKamaev AlexKamaev closed this Nov 29, 2020
@AlexKamaev AlexKamaev reopened this Nov 29, 2020
@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

@testcafe-build-bot
Copy link
Collaborator

1 similar comment
@testcafe-build-bot
Copy link
Collaborator

@AndreyBelym AndreyBelym closed this Dec 1, 2020
@AndreyBelym AndreyBelym reopened this Dec 1, 2020
@testcafe-build-bot
Copy link
Collaborator

@AlexKamaev AlexKamaev marked this pull request as ready for review December 2, 2020 08:35
while (command.cmd === COMMAND.idle) {
await browser.delay(CHECK_STATUS_DELAY);

({ command } = await browser.checkStatus(this.statusUrl, createXHR));
Copy link
Collaborator

Choose a reason for hiding this comment

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

await browser.checkStatus(this.statusUrl, createXHR);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you suggest to remove additional brackets? They are required if we need to destruct objects into already declared variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, it's my mistake. All is OK.

this.statusIndicator.showDisconnection();

throw error;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Which subsystem will handle this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... As I see in the previous version this error was caught and not rethrown. As far as I understand now we rethrow explicitly. @AndreyBelym could you add more details on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is not so much point about it. While debugging, I thought that it would be useful to see an error in the browser console when the browser finally gives up and disconnects. Except for an error in the console, the result is the same as in the previous version - when the browser disconnects, we lost him until someone somehow connects it back manually vi reloading or whatever.

const STATUS_RETRY_DELAY = 1000;
const MAX_STATUS_RETRY = 5;

const SERVICE_WORKER_LOCATION = LOCATION_ORIGIN + `/service-worker.js`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move services routes to constant and use it here.

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 don't get it (
It's already in const, Do you want to extract /service-worker.js to the separate const?

Copy link
Collaborator

Choose a reason for hiding this comment

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


async function tryGetResponse (request) {
try {
return { response: await fetch(request) };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check that fetch is not overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fetch cannot be overridden here, since we are on the idle page

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok

@@ -0,0 +1,47 @@
const MAX_RETRY = 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Write the new files with TypeScript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this moment, only server-side scripts are written in TS. It is not an easy task to enable TS for client scripts in the context of the current issue

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok. Let's make it a separate PR.

Copy link
Contributor Author

@AlexKamaev AlexKamaev Dec 3, 2020

Choose a reason for hiding this comment

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

At this moment, only server-side scripts are written in TS.

this my statement is wrong. However, it is still not easy in the context of this PR. So, yes, I would prefer to make separate PR for this

@@ -64,7 +64,10 @@ testingEnvironments[testingEnvironmentNames.mobileBrowsers] = {
accessKey: process.env.BROWSER_STACK_ACCESS_KEY
},

retryTestPages: true,
ssl: {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you will implement some other changes, could you please remove the ssl and retryTestPages from mobile browsers and macOS + Edge config?

SSL is quite useless without enabling retryTestPages, but we can't enable it right now due to workers are not supported by the current Safari and iOS test devices and we need to debug some complex tests to use newer browsers versions.

@AndreyBelym
Copy link
Contributor

Thank you for a great job and finishing this painful PR

@testcafe-build-bot
Copy link
Collaborator

Copy link
Collaborator

@miherlosev miherlosev left a comment

Choose a reason for hiding this comment

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

If a customer runs the feature in the default way (HTTP, IP as hostname)

testcafe chrome tests --retryTestPages

then it will know that the feature is not working.

Need to inform the customer what the feature is not turning on

The 'retryTestPages' is not turning on.
You need to run TestCafe on the 'secure' host.
To do that:
-- specify the 'localhost' values for the 'hostname' option
-- run TestCafe over HTTPS protocol

and the browser (IE 11) doesn't allow to use of this feature

We are sorry, but you cannot use the `retryTestPages` feature
because the browser doesn't support the 'ServiceWorker' feature.
To start your tests remove this option.

@miherlosev
Copy link
Collaborator

To simplify the review process, it's better to merge this PR now.
@AlexKamaev please make the requested changes in the separate PR.

@miherlosev miherlosev merged commit aa3d748 into DevExpress:master Dec 16, 2020
@Farfurix Farfurix mentioned this pull request Dec 23, 2020
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.

None yet

4 participants