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

Refactored Node test environments to share more #6798

Merged
merged 1 commit into from
Jul 19, 2024

Conversation

kraenhansen
Copy link
Member

What, How & Why?

This is a refactor of the node test environment + the node-based dev environment (running tests without mocha-remote).

@kraenhansen kraenhansen requested a review from gagik July 19, 2024 12:29
@kraenhansen kraenhansen self-assigned this Jul 19, 2024
@cla-bot cla-bot bot added the cla: yes label Jul 19, 2024
Comment on lines -21 to -35
const fs = require("fs-extra");
const path = require("path");
const v8 = require("v8");
const vm = require("vm");

v8.setFlagsFromString("--expose_gc");

const client = new Client({
title: `Node.js v${process.versions.node} on ${os.platform()} (via CommonJS)`,
tests(context) {
// Exposing the Realm constructor as a global
global.fs = fs;
global.path = path;
global.environment = { ...context, node: true };
global.gc = vm.runInNewContext("gc");
Copy link
Member Author

Choose a reason for hiding this comment

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

The main idea behind this refactor is to move this into a shared "node/src/setup-globals.ts" used across these two "runners" and the test ran with bare mocha (without mocha-remote - used when debugging native code).

@@ -16,7 +16,14 @@
//
////////////////////////////////////////////////////////////////////////////

// NOTE: This file is only supposed to be imported from a Node.js environment
if (typeof process.versions.node !== "string") {
throw new Error("This file is only supposed to be imported from a Node.js environment!");
Copy link
Member Author

Choose a reason for hiding this comment

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

Asserting that this gets imported from node instead of simply commenting the intent.

Comment on lines +58 to +60
if (typeof title === "string" && this.parent?.root) {
this.parent.title = title;
}
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 was needed to get the title from "inject-dev-environment.ts" to propagate as expected.

Copy link
Contributor

@gagik gagik left a comment

Choose a reason for hiding this comment

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

LGTM

@kraenhansen kraenhansen merged commit 62195dc into main Jul 19, 2024
31 of 33 checks passed
@kraenhansen kraenhansen deleted the kh/node-test-env-refactored branch July 19, 2024 13:00
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes no-changelog no-jira-ticket Skip checking the PR title for Jira reference T-Internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants