Skip to content

Conversation

@XiNiHa
Copy link
Owner

@XiNiHa XiNiHa commented Apr 7, 2025

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2025

⚠️ No Changeset found

Latest commit: a5d40f8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @XiNiHa, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request focuses on setting up browser and SSR testing environments. It involves modifications to the package.json file to update testing scripts and dependencies, as well as changes to the pnpm-lock.yaml file to reflect the updated dependency tree. The key changes include removing @solidjs/testing-library and happy-dom, while adding @vitest/browser and playwright to facilitate browser testing. The test configuration is updated to use vitest workspaces, separating browser and SSR tests.

Highlights

  • Dependency Updates: The pull request updates the project's dependencies, replacing @solidjs/testing-library and happy-dom with @vitest/browser and playwright to enable browser testing capabilities.
  • Testing Script Modification: The test script in package.json is modified to remove the --ui flag from the vitest command, streamlining the testing process.
  • Vitest Workspaces: The pull request introduces vitest workspaces, separating browser and SSR tests into different configurations. This allows for more targeted and efficient testing.

Changelog

Click here to see the changelog
  • package.json
    • Removes the --ui flag from the test script.
    • Replaces @solidjs/testing-library and happy-dom with @vitest/browser and playwright.
  • pnpm-lock.yaml
    • Updates dependency versions and resolutions to reflect the changes in package.json.
    • Removes entries related to @solidjs/testing-library and happy-dom.
    • Adds entries for @vitest/browser and playwright.
  • tests/createLazyLoadQuery.test.browser.tsx
    • Updates imports to use @vitest/browser/context instead of @solidjs/testing-library.
    • Updates assertions to use playwright's expect instead of @testing-library/jest-dom.
    • Removes renderScreen and uses renderToBody instead.
  • tests/createLazyLoadQuery.test.ssr.tsx
    • Creates a new test file for SSR testing.
  • tests/utils.ts
    • Creates a new file with utility functions for testing.
  • tsconfig.json
    • Adds references to new tsconfig files for browser and SSR tests.
  • tsconfig.test.browser.json
    • Creates a new tsconfig file for browser tests, including playwright types and excluding SSR tests.
  • tsconfig.test.ssr.json
    • Creates a new tsconfig file for SSR tests, excluding browser tests.
  • vitest.config.ts
    • Configures vitest workspaces to separate browser and SSR tests.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


In browser's light, tests now reside,
SSR's realm, where servers confide.
Workspaces defined,
A testing new find,
Bugs tremble, where they can't hide.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request sets up browser and SSR testing using Vitest. It includes changes to package.json, pnpm-lock.yaml, tsconfig.json, and vitest.config.ts, as well as adding a new test file and a utility file. The changes look good overall, but there are a few areas that could be improved.

Summary of Findings

  • Testing Environment Configuration: The configuration for browser and SSR testing environments in vitest.config.ts seems well-structured, but it's crucial to ensure that the conditions and configurations for each environment are correctly set up to avoid conflicts or unexpected behavior during testing.
  • Dependency Updates and Removals: The pnpm-lock.yaml file shows significant dependency updates and removals. It's important to verify that these changes don't introduce any compatibility issues or break existing functionality. Pay close attention to the removal of @solidjs/testing-library and happy-dom and the addition of @vitest/browser and playwright.
  • Browser Test Assertions: The browser tests are updated to use @vitest/browser/context and page for assertions. Ensure that these new assertions are correctly implemented and provide equivalent or better coverage compared to the previous testing library.

Merge Readiness

The pull request introduces significant changes to the testing setup. While the changes seem reasonable, it's crucial to ensure that all tests pass in both browser and SSR environments before merging. I recommend verifying the test coverage and functionality after these changes. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

@XiNiHa XiNiHa force-pushed the ssr-tests branch 2 times, most recently from 7a6eeef to b0ffd19 Compare April 7, 2025 11:11
@pkg-pr-new
Copy link

pkg-pr-new bot commented Apr 7, 2025

Open in StackBlitz

npm i https://pkg.pr.new/XiNiHa/solid-relay@20

commit: 9882da3

@codecov
Copy link

codecov bot commented Apr 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.23%. Comparing base (13cde64) to head (9882da3).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #20      +/-   ##
==========================================
+ Coverage   27.13%   32.23%   +5.10%     
==========================================
  Files          21       21              
  Lines        1360     1461     +101     
  Branches       65       68       +3     
==========================================
+ Hits          369      471     +102     
+ Misses        991      990       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 26, 2025

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
solid-relay a5d40f8 Commit Preview URL

Branch Preview URL
Aug 14 2025, 12:24 PM

Repository owner deleted a comment from cloudflare-workers-and-pages bot May 26, 2025
@claude
Copy link
Contributor

claude bot commented Aug 14, 2025

Claude finished @XiNiHa's task —— View job


Pull Request Review

I'll review this PR that sets up browser and SSR testing infrastructure. Let me analyze the changes systematically:

Todo List:

  • Read and analyze the main configuration files (vitest.config.ts, package.json)
  • Review the testing infrastructure changes and workspace setup
  • Examine the test file migrations and new utilities
  • Check TypeScript configuration changes
  • Evaluate security, performance, and best practices
  • Provide comprehensive feedback and recommendations

Review Feedback

Strengths

  1. Excellent Testing Architecture: The Vitest workspace configuration properly separates browser and SSR testing environments with appropriate resolve conditions.

  2. Clean Migration: Successfully migrated from @solidjs/testing-library + happy-dom to @vitest/browser + playwright with proper API updates.

  3. Proper TypeScript Setup: Well-structured TypeScript project references separating browser and SSR test configurations.

  4. Good Test Coverage: The migrated tests maintain comprehensive coverage including error boundary handling, suspense behavior, and different fetch policies.

🔍 Areas for Improvement

Code Quality & Best Practices

  1. Package.json Dependencies (vitest.config.ts:58, package.json:58)

    • happy-dom is still listed in devDependencies but appears unused after migration
    • Recommendation: Remove happy-dom from package.json to clean up dependencies
  2. Test Script Inconsistency (package.json:33)

    • The --ui flag was removed from the test script, but @vitest/ui is still in devDependencies
    • Recommendation: Either add the --ui flag back or remove the unused UI dependency

Performance Considerations

  1. Browser Test Performance (vitest.config.ts:42)

    • Only Chromium is configured for browser tests, which is good for CI speed
    • Consider adding a separate command for cross-browser testing locally
  2. Parallel Test Execution

    • The workspace configuration allows browser and SSR tests to run in parallel, which is excellent for CI performance

Security Concerns

  1. Playwright Installation (package.json:29)
    • The prepare script runs playwright install which downloads browser binaries
    • Recommendation: Consider using playwright install --with-deps only in CI or document this requirement

Test Coverage & Quality

  1. Missing onTestFinished Import (tests/utils.ts:12)

    • onTestFinished is used but not imported
    • Recommendation: Add import { onTestFinished } from "vitest";
  2. Test Utilities Design (tests/utils.ts:15-37)

    • The renderStream utility is well-designed for SSR testing
    • Good separation between renderToBody for browser tests and renderStream for SSR

Configuration Issues

  1. Environment Configuration (vitest.config.ts:22)
    • Root test environment is set to "node" but workspace browser tests override this properly
    • This is correct but could be clarified with a comment

📋 Minor Issues

  1. Type Safety (tests/createLazyLoadQuery.test.browser.tsx:111)

    • Line 111: page.getByText("name") should likely be page.getByTestId("name") based on the test structure
  2. Test Organization

    • Browser and SSR tests are well-separated with proper file naming conventions
    • Snapshot testing for SSR is appropriately implemented

🎯 Recommendations

  1. Immediate fixes:

    • Remove happy-dom from devDependencies
    • Fix the missing onTestFinished import in tests/utils.ts
    • Consider the test script UI flag consistency
  2. Consider for follow-up:

    • Document the Playwright installation requirement
    • Add cross-browser testing configuration for comprehensive local testing

📊 Overall Assessment

This is a well-executed migration that:

  • ✅ Properly separates browser and SSR testing concerns
  • ✅ Maintains comprehensive test coverage (32.23% coverage increase is excellent)
  • ✅ Uses modern testing tools appropriately
  • ✅ Follows SolidJS and Relay testing best practices

The architecture changes are sound and will provide a solid foundation for testing both browser and SSR scenarios. The identified issues are minor and easily addressable.

Recommendation: ✅ Approve with minor fixes


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.

2 participants