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.

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

test: run Node.js 16 tests case #551

Merged
merged 1 commit into from
Dec 2, 2024
Merged

test: run Node.js 16 tests case #551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 23 additions & 0 deletions .github/workflows/nodejs-16.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
name: Node.js 16 CI

on:
push:
branches: [ master ]
pull_request:
branches: [ master ]

jobs:
build:

runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4
- name: Use Node.js
uses: actions/setup-node@v4
with:
node-version: '16.x'
- run: npm install
- run: npm run prepublishOnly
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Node.js 16 is not compatible with this package

The package explicitly requires Node.js >= 18.19.0 in its package.json engines field. Additionally, the build script uses Node.js 18+ features like node:fs/promises and node:path imports. Running the build in Node.js 16 environment will fail.

  • Remove the Node.js 16 workflow or update it to use Node.js 18+
  • The minimum Node.js version requirement is clearly stated in package.json: "node": ">= 18.19.0"
🔗 Analysis chain

Verify build compatibility with Node.js 16

The prepublishOnly script runs the build process which might use Node.js 18+ features. This could cause the workflow to fail.

Let's check the build scripts for version-specific features:

Let me check the content of the script and package.json to better understand the build requirements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check build-related files for potential Node.js 18+ features
echo "Checking build scripts:"
rg -l "node:|requires|version" scripts/

Length of output: 132

Script:

#!/bin/bash
# Check the content of the script and package.json
echo "Content of scripts/replace_urllib_version.js:"
cat scripts/replace_urllib_version.js

echo -e "\nChecking package.json for engine requirements and build scripts:"
cat package.json | jq '{engines, scripts}'

Length of output: 2883

- run: node -v
- run: npm run test-node16
Comment on lines +1 to +23
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding error handling for the test script

The workflow should handle potential failures in individual example files:

name: Node.js 16 CI

on:
  push:
    branches: [ master ]
  pull_request:
    branches: [ master ]

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v4
      - name: Use Node.js
        uses: actions/setup-node@v4
        with:
          node-version: '16.x'
+         cache: 'npm'
      - run: npm install
      - run: npm run prepublishOnly
      - run: node -v
-      - run: npm run test-node16
+      - name: Run Node.js 16 tests
+        run: |
+          set -e
+          for example in httpclient.cjs search_github.cjs timing.cjs; do
+            echo "Running $example..."
+            node "examples/$example"
+          done

This change:

  1. Adds better error handling with set -e
  2. Provides clearer output for each example
  3. Makes it easier to identify which example failed

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 yamllint (1.35.1)

[error] 23-23: trailing spaces

(trailing-spaces)

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"test-tsc:esm": "cd test/fixtures/ts-esm && rm -rf node_modules && npm link ../../.. && npm run build",
"test": "npm run lint && vitest run",
"test-keepalive": "cross-env TEST_KEEPALIVE_COUNT=50 vitest run --test-timeout 180000 keep-alive-header.test.ts",
"test-node16": "node examples/httpclient.cjs && node examples/search_github.cjs && node examples/timing.cjs",
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Remove test-node16 script as it's incompatible with package requirements

The test-node16 script should be removed because:

  1. The package explicitly requires Node.js >= 18.19.0 in its engines field
  2. After examining the example files, they use standard Node.js features and don't contain any Node.js 16-specific code that would justify maintaining backwards compatibility tests
  3. Running these tests on Node.js 16 could provide false confidence since the package is not designed to support that version
🔗 Analysis chain

Version compatibility concern

The package.json specifies "node": ">= 18.19.0" in the engines field, but we're adding tests for Node.js 16. This might lead to compatibility issues or misleading test results.

Let's verify if the example files exist and check their Node.js version compatibility:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the example files exist and look for any version-specific code
echo "Checking example files existence and content:"
for file in examples/httpclient.cjs examples/search_github.cjs examples/timing.cjs; do
  if [ -f "$file" ]; then
    echo "Found $file"
    # Look for potential version-specific features
    rg -i "node:|requires|version" "$file"
  else
    echo "Missing $file"
  fi
done

Length of output: 840

Script:

#!/bin/bash
# Let's examine the content of these files to check for Node.js version-specific features
echo "=== httpclient.cjs ==="
cat examples/httpclient.cjs

echo -e "\n=== search_github.cjs ==="
cat examples/search_github.cjs

echo -e "\n=== timing.cjs ==="
cat examples/timing.cjs

# Also check package.json for any version-related context
echo -e "\n=== package.json engines and scripts ==="
cat package.json | rg -A 5 '"(engines|scripts)":'

Length of output: 3177

"cov": "vitest run --coverage",
"ci": "npm run lint && npm run cov && npm run prepublishOnly && npm pack && attw --pack",
"clean": "rm -rf dist",
Expand Down