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

fix(ct): generate url with "runUuid" as pathname #912

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

DudaGod
Copy link
Member

@DudaGod DudaGod commented Apr 19, 2024

What is done

Component tests use vite, which generates for each request index.html. But in the case of parallel launch of several browsers, a case may arise when the same output is returned to both browsers, although the urls are different by query parameter.

I create an issue to vite - vitejs/vite#16467. As a result I found that vite cached response for each request by pathname (query parameters are not involved). So for now urls modified from http://localhost:4444/?runUuid=12345 to http://localhost:4444/12345. After that, the problem does not reproduce anymore.

@@ -1,6 +1,7 @@
import path from "node:path";
import url from "node:url";
import { builtinModules } from "node:module";
import _ from "lodash";
Copy link
Member Author

Choose a reason for hiding this comment

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

This plugin executes in nodejs env so there are no problems with import all methods from module.

@@ -176,7 +176,7 @@ export class TestRunner extends NodejsEnvTestRunner {
});

const timeout = this._config.urlHttpTimeout || this._config.httpTimeout;
const uri = new URI(this._config.baseUrl).query({ runUuid: this._runUuid }).toString();
const uri = urljoin(this._config.baseUrl, this._runUuid);
Copy link
Member

Choose a reason for hiding this comment

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

It would be unclear, what that number means.

Perhaps, we should do something like this:

const uri = urljoin(this._config.baseUrl, `run-uuid-${this._runUuid}`);

So we get something like http://foo.bar/run-uuid-12345

Copy link
Member Author

Choose a reason for hiding this comment

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

we agreed to use an URL like this /run-uuids/:uuid in the chat

@@ -59,21 +60,17 @@ export const plugin = async (): Promise<Plugin[]> => {
}

const urlParsed = url.parse(req.originalUrl);
const urlParamString = new URLSearchParams(urlParsed.query || "");
const runUuid = _.compact(urlParsed.pathname?.split("/"))[0];
Copy link
Member

Choose a reason for hiding this comment

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

And here we could look for run-uuid-\d+ path component, instead of just taking the first one

Copy link
Member

@KuznetsovRoman KuznetsovRoman left a comment

Choose a reason for hiding this comment

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

I would replace this._runUuid in pathname with something like run-uuid-${this._runUuid}

@DudaGod DudaGod force-pushed the HERMIONE-1521.fix_gen_index.html branch from ccd6589 to 6c46969 Compare April 22, 2024 11:44
@@ -35,7 +36,6 @@ export const setupTransformHook = (): VoidFunction => {
},
}),
],
require("@babel/plugin-transform-typescript"),
Copy link
Member Author

Choose a reason for hiding this comment

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

In some cases @babel/plugin-transform-typescript works incorrectly with my tsx files. But if I use @babel/preset-typescript then everything works fine. I know that this preset just use this plugin but it looks like preset run after plugins due to the docs. So I just rewrite code to use preset instead this plugin.

@DudaGod DudaGod merged commit c8fcd87 into master Apr 22, 2024
2 checks passed
@DudaGod DudaGod deleted the HERMIONE-1521.fix_gen_index.html branch April 22, 2024 11:57
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

2 participants