Skip to content

Conversation

@Kr0san89
Copy link
Contributor

@Kr0san89 Kr0san89 commented Nov 18, 2025

node24 does not accept certificates which are expired, bypass the reject by the added option

Acceptance Checklist

  • Story: Code is focused on the linked stories and solves a problem
  • One of:
    • For Bugs: A unit test is added or an existing one modified
    • For Features: New unit tests are added covering the new functions or modifications
  • Code Documentation changes are included for public interfaces and important / complex additions
  • External Documentation is included for API changes, or other external facing interfaces

Review Checklist

  • The code does not duplicate existing functionality that exists elsewhere
  • The code has been linted and follows team practices and style guidelines
  • The changes in the PR are relevant to the title
    • changes not related should be moved to a different PR
  • All errors or error handling is actionable, and informs the viewer on how to correct it

Summary by CodeRabbit

  • Tests

    • Updated test cleanup procedures for improved compatibility.
  • Chores

    • Enhanced TLS configuration for explicit and implicit server contexts.

node24 does not accept certificates which are expired, bypass the reject
@Kr0san89 Kr0san89 requested a review from a team as a code owner November 18, 2025 21:54
@coderabbitai
Copy link

coderabbitai bot commented Nov 18, 2025

Walkthrough

Updated tests to replace deprecated directory removals (fs.rmdirSyncfs.rmSync) and to add a ciphers: "DEFAULT:@SECLEVEL=0" property to TLS options for explicit and IMPLICIT server setups in integration tests.

Changes

Cohort / File(s) Summary
Filesystem cleanup in tests
test/index.spec.js
Replaced fs.rmdirSync(...) calls with fs.rmSync(...), preserving recursive removal semantics (calls updated both with and without explicit { recursive: true }).
TLS test configuration
test/index.spec.js
Added ciphers: "DEFAULT:@SECLEVEL=0" to the TLS options object for explicit and IMPLICIT TLS server setups (trailing comma present in object literal).

Sequence Diagram(s)

(omitted — changes are configuration and test cleanup only, no control-flow or feature-level modifications warranting a sequence diagram)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check test/index.spec.js for all occurrences of rmdirSync → ensure none remain.
  • Verify ciphers insertion is applied consistently to both explicit and IMPLICIT TLS contexts and that trailing comma is syntactically acceptable in the project style.
  • Confirm tests run on CI/local environment with the lowered SECLEVEL and that this change is intentional for test compatibility.

Suggested reviewers

  • iamart

Poem

🐰 I nibble at bugs, then hop to the test,
rmSync clears paths so folders can rest,
TLS ciphers loosen their strict little bow,
SECLEVEL lowered — the rabbit says go! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description addresses the core issue (Node 24 rejecting expired certificates) and the solution (adding TLS cipher option), but all checklist items remain unchecked, suggesting incomplete review preparation. Complete the acceptance and review checklists by checking relevant items or explaining why they don't apply to this maintenance change.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main objective: getting tests running with Node 24, which aligns with the two key changes (fs API update and TLS cipher configuration).
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1309c5d and 1ac6850.

📒 Files selected for processing (1)
  • test/index.spec.js (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/index.spec.js (2)
test/fs.spec.js (1)
  • fs (9-9)
test/start.js (1)
  • fs (2-2)
🔇 Additional comments (3)
test/index.spec.js (3)

108-108: LGTM: Correct migration to fs.rmSync.

The update from deprecated fs.rmdirSync to fs.rmSync is the correct API change for Node.js 14+ compatibility.


339-339: LGTM: Consistent migration to fs.rmSync.

All test cleanup operations correctly use the updated fs.rmSync API instead of the deprecated fs.rmdirSync.

Also applies to: 351-351, 358-358


484-484: LGTM: Appropriate cipher configuration for test certificates.

Setting @SECLEVEL=0 correctly allows the tests to use expired certificates. This is appropriate for a test environment and directly addresses the PR objective for Node 24 compatibility.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
test/index.spec.js (1)

474-485: TLS ciphers override is reasonable for tests; consider keeping EXPLICIT/IMPLICIT configs aligned

The added ciphers: "DEFAULT:@SECLEVEL=0" on the test server’s tls options is a pragmatic way to keep the legacy test certs working on newer Node/OpenSSL stacks while still scoping the weaker settings to test-only code. No functional issues here from the test perspective.

As a small future-proofing tweak, you might want to mirror the same ciphers override in the skipped #IMPLICIT suite’s tls config so it doesn’t unexpectedly start failing on Node 24+ if someone re-enables those tests later.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6a3b227 and 1309c5d.

📒 Files selected for processing (1)
  • test/index.spec.js (1 hunks)

@Kr0san89 Kr0san89 marked this pull request as draft November 18, 2025 21:57
@Kr0san89 Kr0san89 marked this pull request as ready for review November 18, 2025 22:05
@Kr0san89
Copy link
Contributor Author

@iamart that should fix all problems regarding the build & node 24

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.

1 participant