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: suppress experimental warning for node 23 MONGOSH-1895 #2258

Merged
merged 16 commits into from
Nov 12, 2024
Merged
8 changes: 4 additions & 4 deletions .evergreen.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7132,7 +7132,7 @@ functions:
{
export NODE_JS_VERSION=${node_js_version}
source .evergreen/setup-env.sh
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently, PUPPETEER_SKIP_CHROMIUM_DOWNLOAD had been deprecated and it seems to have been removed altogether (couldn't find any reference to it in the puppeteer version we're using). See puppeteer/puppeteer#11069.

npm run evergreen-release draft
}

Expand All @@ -7150,7 +7150,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh download-and-list-artifacts
- command: shell.exec
params:
Expand Down Expand Up @@ -7179,7 +7179,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh publish -- --dry-run

release_publish:
Expand All @@ -7198,7 +7198,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh publish

run_perf_tests:
Expand Down
2 changes: 1 addition & 1 deletion .evergreen/compile-artifact.sh
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ elif [ -n "$MONGOSH_SHARED_OPENSSL" ]; then
export LD_LIBRARY_PATH=/tmp/m/opt/lib
fi

export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
npm run evergreen-release compile
dist/mongosh --version
dist/mongosh --build-info
Expand Down
8 changes: 4 additions & 4 deletions .evergreen/evergreen.yml.in
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@ functions:
{
export NODE_JS_VERSION=${node_js_version}
source .evergreen/setup-env.sh
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
npm run evergreen-release draft
}

Expand All @@ -985,7 +985,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh download-and-list-artifacts
- command: shell.exec
params:
Expand Down Expand Up @@ -1014,7 +1014,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh publish -- --dry-run

release_publish:
Expand All @@ -1033,7 +1033,7 @@ functions:
node_js_version: ${node_js_version}
script: |
set -e
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
.evergreen/run-evergreen-release.sh publish

run_perf_tests:
Expand Down
2 changes: 1 addition & 1 deletion .evergreen/package-and-upload-artifact.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ if [ "$(uname)" == Linux ]; then
cp "$(pwd)/../tmp/expansions.yaml" tmp/expansions.yaml
(cd scripts/docker && bash "$BASEDIR/retry-with-backoff.sh" docker build -t rocky8-package -f rocky8-package.Dockerfile .)
echo Starting Docker container packaging
docker run -e PUPPETEER_SKIP_CHROMIUM_DOWNLOAD \
docker run -e PUPPETEER_SKIP_DOWNLOAD \
-e EVERGREEN_EXPANSIONS_PATH=/tmp/build/tmp/expansions.yaml \
-e NODE_JS_VERSION \
-e PACKAGE_VARIANT \
Expand Down
2 changes: 1 addition & 1 deletion .evergreen/run-evergreen-release.sh
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@ echo "//registry.npmjs.org/:_authToken=${devtoolsbot_npm_token}" > .npmrc
set -x
export NODE_JS_VERSION=${node_js_version}
source .evergreen/setup-env.sh
export PUPPETEER_SKIP_CHROMIUM_DOWNLOAD="true"
export PUPPETEER_SKIP_DOWNLOAD="true"
npm run evergreen-release $@
39 changes: 39 additions & 0 deletions .github/workflows/homebrew.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
name: "Smoke Test Homebrew install"

on:
schedule:
- cron: "0 0 * * *"
pull_request: # TODO: remove pull_request trigger
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting on IT to create the webhook, after which I'll clean up the TODOs in this file.


jobs:
smoke-test-homebrew:
name: Test on ${{ matrix.runner }}
runs-on: ${{ matrix.runner}}
strategy:
matrix:
runner: [macos-13, macos-14, macos-15]
fail-fast: false
steps:
- run: brew install mongosh
- run: mongosh --smokeTests
- name: Report failure
# TODO: replace with failure()
if: ${{ success() }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Open TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll fix that before we merge - just wanted to test it first once I get the slack webhook from IT.

env:
SLACK_WEBHOOK_URL: ${{ secrets.SLACK_MONGOSH_DEVEL_WEBHOOK_URL }}
SLACK_WEBHOOK_TYPE: INCOMING_WEBHOOK
uses: slackapi/[email protected]
with:
payload: |
{
"text": "Homebrew smoke test failed on macOS ${{ matrix.runner }}"
"blocks": [
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": "Failed run: [${{ github.run_id }}](${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }})"
}
}
]
}
36 changes: 36 additions & 0 deletions .github/workflows/smoke-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: "Run Smoke Tests"
on:
push:
branches:
- main
pull_request:

jobs:
smoke-tests:
name: "OS: ${{ matrix.runner }}, node@${{ matrix.node }}"
strategy:
matrix:
runner: [ubuntu, macos, windows]
node: [20.x, 22.x, 23.x]
fail-fast: false
runs-on: ${{ matrix.runner }}-latest
timeout-minutes: 15
env:
npm_config_loglevel: verbose
npm_config_foreground_scripts: "true"
PUPPETEER_SKIP_DOWNLOAD: "true"
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node }}
cache: "npm"

- name: Install npm@10
run: npm install -g npm@10

- name: Install dependencies
run: npm ci

- name: Run smoke tests
run: npm run test-smoke
nirinchev marked this conversation as resolved.
Show resolved Hide resolved
6 changes: 4 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"author": "Compass Team <[email protected]>",
"scripts": {
"bootstrap-with-chromium": "npm install && npm run compile",
"bootstrap": "npx cross-env PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=1 npm install && npm run compile",
"bootstrap": "npx cross-env PUPPETEER_SKIP_DOWNLOAD=1 npm install && npm run compile",
"clean": "lerna clean -y && rm -Rf node_modules",
"check": "lerna run check --since HEAD --exclude-dependents",
"check-ci": "npm run check --workspaces --if-present",
Expand All @@ -26,6 +26,8 @@
"test-evergreen-expansions": "bash .evergreen/compilation-context-expansions.test.sh",
"replace-package": "node scripts/replace-package.js",
"test-nodedriver": "bash .evergreen/test-node-driver.sh",
"pretest-smoke": "npm run compile-cli",
"test-smoke": "npm run test-smoke -w @mongosh/cli-repl",
"compile": "npm run compile --workspaces --if-present",
"compile-cli": "lerna run compile --scope @mongosh/cli-repl --include-dependencies",
"prestart-cli": "npm run compile-cli",
Expand Down Expand Up @@ -167,4 +169,4 @@
"overrides": {
"cookie": "^0.7.2"
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing newline at end of file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, will need to take a look at my vscode settings - I thought it was prettier that removes it 😬

1 change: 1 addition & 0 deletions packages/cli-repl/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"test-coverage": "nyc --no-clean --cwd ../.. --reporter=none npm run test",
"test-ci-coverage": "nyc --no-clean --cwd ../.. --reporter=none npm run test-ci",
"test-apistrict-ci": "cross-env MONGOSH_TEST_FORCE_API_STRICT=1 npm run test-ci",
"test-smoke": "node bin/mongosh.js --smokeTests",
"eslint": "eslint",
"lint": "npm run eslint . && npm run prettier -- --check .",
"check": "npm run lint && npm run depcheck",
Expand Down
30 changes: 30 additions & 0 deletions packages/cli-repl/src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ if ((v8 as any)?.startupSnapshot?.isBuildingSnapshot?.()) {
// eslint-disable-next-line complexity
async function main() {
markTime(TimingCategories.Main, 'entered main');
suppressExperimentalWarnings();
if (process.env.MONGOSH_RUN_NODE_SCRIPT) {
// For uncompiled mongosh: node /path/to/this/file script ... -> node script ...
// FOr compiled mongosh: mongosh mongosh script ... -> mongosh script ...
Expand Down Expand Up @@ -311,3 +312,32 @@ async function ask(prompt: string): Promise<string> {
process.stdin.unpipe(stdinCopy);
}
}

/**
* Helper to suppress experimental warnings emitted by node if necessary.
*
* In Node.js 23 require()ing ESM modules will work, but emit an experimental warning like
* CommonJS module ABC is loading ES Module XYZ using require(). This is causing problems for
* the way we import fetch - see relevant comments here:
* https://github.com/mongodb-js/devtools-shared/blob/29ceeb5f51d29883d4a69c83e68ad37b0965d49e/packages/devtools-proxy-support/src/fetch.ts#L12-L17
*/
function suppressExperimentalWarnings() {
const nodeMajorVersion = process.versions.node.split('.').map(Number)[0];
if (nodeMajorVersion >= 23) {
const originalEmit = process.emitWarning;
process.emitWarning = (warning, ...args: any[]): void => {
if (args[0] === 'ExperimentalWarning') {
return;
}

if (
typeof args[0] === 'object' &&
args[0].type === 'ExperimentalWarning'
) {
return;
}

originalEmit(warning, ...args);
nirinchev marked this conversation as resolved.
Show resolved Hide resolved
};
}
}
5 changes: 3 additions & 2 deletions packages/cli-repl/src/smoke-tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,7 @@ async function runSmokeTest({
// eslint-disable-next-line
const { spawn } = require('child_process') as typeof import('child_process');
const proc = spawn(executable, [...args], {
stdio: ['pipe', 'pipe', includeStderr ? 'pipe' : 'inherit'],
stdio: 'pipe',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing to pipe indiscriminately so that we can then validate there's no warnings in stderr.

});
let stdout = '';
let stderr = '';
Expand All @@ -420,13 +420,14 @@ async function runSmokeTest({
input,
output,
stdout,
stderr,
stderr: includeStderr ? stderr : '',
executable,
actualExitCode,
args: args.map((arg) => redactURICredentials(arg)),
};
try {
assert.match(includeStderr ? `${stdout}\n${stderr}` : stdout, output);
assert.doesNotMatch(stderr, /ExperimentalWarning/);
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'm tempted to change this to match more broadly just Warning, but I'm not sure if that might result in false positives.

if (exitCode !== undefined) {
assert.strictEqual(actualExitCode, exitCode);
}
Expand Down
Loading