Skip to content

No codegen-reference to givePort in tests#3089

Merged
RobinTail merged 1 commit intomasterfrom
no-codegen-giveport
Nov 24, 2025
Merged

No codegen-reference to givePort in tests#3089
RobinTail merged 1 commit intomasterfrom
no-codegen-giveport

Conversation

@RobinTail
Copy link
Copy Markdown
Owner

@RobinTail RobinTail commented Nov 24, 2025 •
edited by coderabbitai Bot
Loading

This should simplify CJS test without tsx and overall it's redundant

Summary by CodeRabbit

  • Chores
    • Optimized internal test generation tooling by streamlining program assembly logic and updating test content mappings for consistency. No user-facing changes.

✏️ Tip: You can customize this high-level summary in your review settings.

@RobinTail RobinTail added refactoring The better way to achieve the same result prevention labels Nov 24, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Nov 24, 2025 •
edited
Loading

Walkthrough

The changes refactor test program generation by computing the program directly from extractQuickStartFromReadme() instead of maintaining a separate constant, and update placeholder substitutions to use evaluated function calls rather than literal string values.

Changes

Cohort / File(s) Summary
Test Generation Refactoring
tools/make-tests.ts		 Consolidates program computation by removing the separate quickStart constant, replaces literal port placeholder strings ("givePort('cjs')") with evaluated function calls (String(givePort("cjs"))), and updates issue952 mapping to use program replacement instead of quickStart replacement.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Single file modification with consistent pattern changes across multiple substitutions
  • Logic remains functionally equivalent; primarily consolidation and placeholder evaluation updates
  • Verify that extractQuickStartFromReadme() correctly replaces the removed quickStart constant functionality
  • Confirm port placeholder substitutions now produce identical string values to previous behavior

Possibly related PRs

Poem

🐰 Constants consolidated, functions now flow,
Port placeholders dance, evaluated just so,
One file refined with a whimsical touch,
Test generation sings—refactored with much! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'No codegen-reference to givePort in tests' directly and clearly describes the main change: removing codegen references to givePort from test files, which aligns with the PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch no-codegen-giveport
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between df3ebb4 and 46c9309.

📒 Files selected for processing (1)
  • tools/make-tests.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings 
Learnt from: RobinTail
Repo: RobinTail/express-zod-api PR: 2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:08:56.475Z
Learning: The `cjs-test` directory in the express-zod-api repository is a test workspace and should be excluded when checking for main project version consistency with changelog entries.
📚 Learning: 2025-05-27T19:30:51.885Z 
Learnt from: RobinTail
Repo: RobinTail/express-zod-api PR: 2546
File: compat-test/sample.ts:1-1
Timestamp: 2025-05-27T19:30:51.885Z
Learning: Files in compat-test/ directories, especially those named sample.ts or similar, are often test fixtures for migration scripts and may intentionally contain deprecated or "incorrect" code that the migration tooling is designed to fix. These should not be flagged as issues.

Applied to files:

  • tools/make-tests.ts
📚 Learning: 2025-06-02T21:08:56.475Z 
Learnt from: RobinTail
Repo: RobinTail/express-zod-api PR: 2697
File: CHANGELOG.md:5-5
Timestamp: 2025-06-02T21:08:56.475Z
Learning: The `cjs-test` directory in the express-zod-api repository is a test workspace and should be excluded when checking for main project version consistency with changelog entries.

Applied to files:

  • tools/make-tests.ts
📚 Learning: 2025-10-02T17:42:48.840Z 
Learnt from: RobinTail
Repo: RobinTail/express-zod-api PR: 0
File: :0-0
Timestamp: 2025-10-02T17:42:48.840Z
Learning: In express-zod-api v25 (ESM-only), the `.example()` method error occurs when user code runs as CommonJS. Express Zod API patches Zod's ESM bundle with `.example()`, but CommonJS code requires a separate CJS bundle instance that lacks this patch. Users must run their code as ESM by: (1) setting `"type": "module"` in package.json, (2) using `.mts` or `.mjs` file extensions, or (3) using tools like `tsx` or `vite-node` that provide their own ESM-compatible compilation.

Applied to files:

  • tools/make-tests.ts
📚 Learning: 2025-05-27T19:35:57.357Z 
Learnt from: RobinTail
Repo: RobinTail/express-zod-api PR: 2546
File: express-zod-api/tests/buffer-schema.spec.ts:32-37
Timestamp: 2025-05-27T19:35:57.357Z
Learning: In the express-zod-api project, tests are run from the `express-zod-api` workspace directory, and the project uses an ESM-first environment without `__dirname`. Relative paths like `../logo.svg` in test files correctly resolve to the repository root due to this test execution context.

Applied to files:

  • tools/make-tests.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: report
🔇 Additional comments (3)
tools/make-tests.ts (3)

17-19: LGTM! Clean refactoring to embed port numbers at generation time.

The direct computation of program and examplePort is cleaner than maintaining separate constants. Evaluating givePort() here and embedding the results as strings achieves the PR objective of removing givePort references from generated test files, which simplifies the CJS test setup.

22-24: LGTM! Unique port assignment for each test variant.

Each test variant correctly receives its own unique port number, which prevents conflicts during parallel test execution.

25-25: Ignore this review comment — the concerns do not apply to issue952-test.

The original review misidentifies the purpose of issue952-test. This is a TypeScript compilation test (tsc -p tsconfig.json), not a runtime server test like cjs-test and esm-test.

Port conflict concern (invalid): issue952-test generates code with listen: 8090 but never executes it at runtime. There is no server listening on any port, so no conflict can occur.

Broad regex concern (intentional): The /const/g replacement is the entire purpose of this regression test. It tests a "Dedicated environment for Issue #952", which was about insufficient exports. The test verifies that converting const to export const produces valid, compilable code with proper TypeScript declarations.

Likely an incorrect or invalid review comment.

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.

@coveralls-official
Copy link
Copy Markdown

coveralls-official Bot commented Nov 24, 2025

Coverage Status

coverage: 100.0%. remained the same
when pulling 46c9309 on no-codegen-giveport
into df3ebb4 on master.

@RobinTail RobinTail merged commit bdc13ab into master Nov 24, 2025
13 checks passed
@RobinTail RobinTail deleted the no-codegen-giveport branch November 24, 2025 16:54
@coderabbitai coderabbitai Bot mentioned this pull request Nov 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

refactoring The better way to achieve the same result

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@RobinTail