Skip to content

tests: use the same browser instance across tests and better use explicit resource management #2901

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.

Sign up for GitHub

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

Merged
marvinhagemeister merged 10 commits into from
May 21, 2025
Merged

tests: use the same browser instance across tests and better use explicit resource management #2901

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fe24c7b
test: use the same browser instance across tests
iuioiua May 7, 2025
72c973e
work
iuioiua May 8, 2025
3049b2f
tweak
iuioiua May 8, 2025
a3c24a1
work
iuioiua May 8, 2025
8364d95
tweak
iuioiua May 8, 2025
8a62637
Merge branch 'main' into browser-instance
iuioiua May 8, 2025
beaeea3
fix
iuioiua May 12, 2025
754bed1
Merge branch 'main' into browser-instance
iuioiua May 12, 2025
ccc4286
Merge branch 'main' into browser-instance
iuioiua May 18, 2025
52c3a53
Merge branch 'main' into browser-instance
marvinhagemeister May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion deno.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"@std/semver": "jsr:@std/semver@1",
"@std/streams": "jsr:@std/streams@1",

"@astral/astral": "jsr:@astral/astral@^0.5.2",
"@astral/astral": "jsr:@astral/astral@^0.5.3",
"@marvinh-test/fresh-island": "jsr:@marvinh-test/fresh-island@^0.0.1",
"linkedom": "npm:linkedom@^0.16.11",
"@std/async": "jsr:@std/async@1",
Expand Down
107 changes: 22 additions & 85 deletions deno.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

43 changes: 14 additions & 29 deletions tests/test_utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { App, getIslandRegistry, setBuildCache } from "../src/app.ts";
import { launch, type Page } from "@astral/astral";
import * as colors from "@std/fmt/colors";
import { type Document, DOMParser, HTMLElement } from "linkedom";
import { DOMParser, HTMLElement } from "linkedom";
import { Builder } from "../src/dev/builder.ts";
import { TextLineStream } from "@std/streams/text-line-stream";
import * as path from "@std/path";
Expand All @@ -28,6 +28,16 @@ import { NodeProcess } from "./fixtures_islands/NodeProcess.tsx";
import { FreshAttrs } from "./fixtures_islands/FreshAttrs.tsx";
import { OptOutPartialLink } from "./fixtures_islands/OptOutPartialLink.tsx";

const browser = await launch({
args: [
"--window-size=1280,720",
...((Deno.env.get("CI") && Deno.build.os === "linux")
? ["--no-sandbox"]
Copy link
Contributor

@lino-levan lino-levan May 7, 2025

Choose a reason for hiding this comment

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

https://github.com/lino-levan/astral?tab=readme-ov-file#no-usable-sandbox-with-user-namespace-cloning-enabled

I would recommend removing app armor instead of enabling --no-sandbox

Copy link
Contributor Author

@iuioiua iuioiua May 7, 2025

Choose a reason for hiding this comment

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

What is the advantage? Genuine question.

Copy link
Contributor Author

@iuioiua iuioiua May 7, 2025

Choose a reason for hiding this comment

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

Or do you think that's the issue here?

Copy link
Contributor

@lino-levan lino-levan May 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The biggest reason is it cleans up the codebase. I've heard of really obscure bugs not being caught in CI because of --no-sandbox, but I've never seen one myself so take that with a grain of salt.

: []),
],
headless: true,
});

export function getIsland(pathname: string) {
return path.join(
import.meta.dirname!,
Expand Down Expand Up @@ -79,45 +89,23 @@ export async function withBrowserApp(
fn: (page: Page, address: string) => void | Promise<void>,
) {
const aborter = new AbortController();
const server = Deno.serve({
await using server = Deno.serve({
hostname: "localhost",
port: 0,
signal: aborter.signal,
onListen: () => {}, // Don't spam terminal with "Listening on..."
}, app.handler());

const browser = await launch({
args: [
"--window-size=1280,720",
...((Deno.env.get("CI") && Deno.build.os === "linux")
? ["--no-sandbox"]
: []),
],
headless: true,
});

const page = await browser.newPage();
try {
await using page = await browser.newPage();
await fn(page, `http://localhost:${server.addr.port}`);
} finally {
await page.close();
await browser.close();
aborter.abort();
await server?.finished;
}
}

export async function withBrowser(fn: (page: Page) => void | Promise<void>) {
const browser = await launch({
args: [
"--window-size=1280,7201",
...((Deno.env.get("CI") && Deno.build.os === "linux")
? ["--no-sandbox"]
: []),
],
headless: true,
});
const page = await browser.newPage();
await using page = await browser.newPage();
try {
await fn(page);
} catch (err) {
Expand All @@ -127,9 +115,6 @@ export async function withBrowser(fn: (page: Page) => void | Promise<void>) {
// deno-lint-ignore no-console
console.log(html);
throw err;
} finally {
await page.close();
await browser.close();
}
}

Expand Down
Loading