Fix incremental render tests

[AbanoubGhadban]

Summary

fixes #2031

Remove this paragraph and provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Pull Request checklist

Remove this line after checking all the items here. If the item is not applicable to the PR, both check it out and wrap it by ~.

• Add/update test to cover these changes
• Update documentation
• Update CHANGELOG file

Add the CHANGELOG entry at the top of the file.

Other Information

Remove this paragraph and mention any other important and relevant information such as benchmarks.

---

This change is 

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch abanoub/fix-incremental-render-tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

Code Review

Thank you for working on fixing the incremental render tests! Here's my review:

✅ Positive Aspects

1. Excellent Test Isolation: Creating separate bundle-incremental.js and secondary-bundle-incremental.js fixtures is the right approach. This properly isolates the incremental rendering functionality from standard tests.

2. Clear Problem Statement: The commit message clearly explains the issue - the async/stream helpers added to the original fixtures broke existing tests in vm.test.ts, handleRenderRequest.test.ts, and worker.test.ts.

3. Backward Compatibility: Restoring the original fixtures to their minimal 3-line versions preserves backward compatibility with existing tests.

4. Type Safety Improvements: Exporting RequestBody interface and renaming ProtocolVersionBody to RequestBody improves type consistency.

---

⚠️ Issues to Address

1. CRITICAL: Focused Test in incrementalRender.test.ts:677

test.only('basic incremental update - initial request gets value, update chunks set value', async () => {

The .only modifier means only this one test will run, skipping all other tests in the suite. This will cause CI failures.

Fix: Remove .only and change to:

test('basic incremental update - initial request gets value, update chunks set value', async () => {

2. Test Refactoring Concerns in incrementalRender.test.ts

The PR removes several important test cases without clear justification:

• incremental updates work with multiple bundles using runOnOtherBundle (lines 712-766 removed)
• error handling in incremental render updates (lines 768-832 removed)
• update chunks with non-existent bundle timestamp (lines 834-888 removed)

Questions:

• Were these tests failing and need to be rewritten?
• Are they redundant with other test coverage?
• Should they be preserved but adapted to use the new incremental fixtures?

Recommendation: If these tests are still needed, they should be updated to use createIncrementalVmBundle() rather than deleted. If they're no longer needed, please add a comment explaining why.

3. Incomplete Test Updates

Looking at the remaining test at line 757 (complex multi-bundle streaming scenario), it correctly uses the new helpers:

await createIncrementalVmBundle(TEST_NAME);
await createIncrementalSecondaryVmBundle(TEST_NAME);

But the deleted test at line 677 (before the fix) still used the old createVmBundle(). Ensure all incremental render tests consistently use the incremental fixtures.

4. Response Structure Change Not Documented

In handleRenderRequest.test.ts, all assertions changed from:

expect(result).toEqual(renderResult);

to:

expect(result.response).toEqual(renderResult);

Question: This suggests handleRenderRequest now returns an object with a response property instead of the response directly. Is this intentional? If so, what else is in the returned object and why was this changed?

Recommendation: If this is part of the incremental rendering changes, it should be documented in the PR description.

5. Gemfile.lock Changes

The Gemfile.lock includes new dependencies (async, console, fiber-annotation, etc.). These appear unrelated to the test fixture changes.

Questions:

• Are these dependencies required for incremental rendering functionality?
• Were they added accidentally during development?
• Should they be in a separate PR?

6. Ruby Spec Changes Need Clarity

The request_spec.rb changes show that bundle uploads now go through a separate /upload-assets endpoint rather than being included in the render request. This is a significant architectural change.

Recommendation: This should be clearly documented in the PR description as it's a behavioral change beyond just "fixing tests".

---

📋 Pull Request Checklist Items

Per the PR template, please ensure:

• Add/update tests: ✅ Tests are being fixed, but verify deleted tests don't need to be preserved
• Update documentation: ❌ The architectural changes (response structure, upload-assets endpoint) should be documented
• Update CHANGELOG: ❌ Not checked in the PR - run /update-changelog if this includes user-visible changes

---

🔧 Required Actions Before Merge

1. Remove .only from test (critical - will break CI)
2. Explain or restore deleted tests (62, 60, and 54 lines removed respectively)
3. Document the response structure change in handleRenderRequest
4. Explain Gemfile.lock changes or remove if unrelated
5. Document the upload-assets endpoint change if it's a real behavioral change
6. Run linting: bundle exec rubocop (per CLAUDE.md requirements)
7. Verify all tests pass: rake run_rspec

---

💡 Additional Recommendations

1. Helper Function Naming: Consider renaming to make the distinction clearer:

   • createVmBundle → createBasicVmBundle
   • createIncrementalVmBundle → keeps current name

   This makes it explicit that there are two types of fixtures for different use cases.

2. Test Organization: Consider grouping all incremental render tests to use a shared setup that calls createIncrementalVmBundle in beforeEach, similar to how you added resetForTest in the beforeEach.

3. Fixture Documentation: Add a comment at the top of bundle-incremental.js explaining:

   • Why it exists separately from bundle.js
   • What functionality it supports
   • Which tests use it

---

Summary

The core approach of separating incremental fixtures is excellent. The main concerns are:

1. The .only test modifier (blocking issue)
2. Unexplained deletion of test cases
3. Undocumented architectural changes in response structure and upload flow

Once these are addressed, this should be ready to merge. Great work on isolating the test concerns! 🎉