E2e UI tests#10
Conversation
…he existing Playwright config
… it now runs the smoke test only on Chromium
…e:index, so calling npm test now covers the Chromium smoke check as well
… ESLint no longer flags the smoke check
Summary of ChangesHello @cs-util, 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! This pull request focuses on enhancing the quality and usability of the project's documentation and testing infrastructure. It standardizes code style within documentation, particularly by enforcing single quotes and improving code block formatting. Additionally, it refines guidelines for code coverage ignores and provides clearer instructions for setting up various Three.js testing environments, ensuring that developers have up-to-date and consistent resources. A new dedicated script for running specific end-to-end tests has also been introduced to streamline the testing workflow. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request does a great job of improving documentation clarity and standardizing code style, particularly by enforcing the use of single quotes and improving formatting. The changes make the documentation easier to read and maintain. I've found a few minor inconsistencies with the single-quote rule in the documentation files that could be addressed. Additionally, I've identified a potential issue in the Playwright test where a fixed timeout has been added, which is generally considered an anti-pattern and could lead to test flakiness. My detailed comments provide suggestions to resolve these points.
| await page.waitForLoadState('networkidle', { timeout: 10000 }); | ||
| } catch (error) { | ||
| } catch { | ||
| await page.waitForLoadState('load'); |
There was a problem hiding this comment.
Adding a fixed waitForTimeout is an anti-pattern in Playwright tests as it can lead to flaky and slower tests. It's often a temporary fix for race conditions. Instead of a fixed wait, it's better to rely on Playwright's web-first assertions and auto-waiting capabilities to ensure the page is in the desired state. For example, you could wait for a specific element that indicates the page has fully loaded before checking for console errors. Please consider removing this line.
| "istanbul ignore next", | ||
| "istanbul ignore if", | ||
| "istanbul ignore else", | ||
| "istanbul ignore file", | ||
| "c8 ignore next" |
There was a problem hiding this comment.
The strings in the terms array are still using double quotes. This is inconsistent with the goal of this pull request to standardize on single quotes. For consistency, these should be updated to use single quotes.
| "istanbul ignore next", | |
| "istanbul ignore if", | |
| "istanbul ignore else", | |
| "istanbul ignore file", | |
| "c8 ignore next" | |
| 'istanbul ignore next', | |
| 'istanbul ignore if', | |
| 'istanbul ignore else', | |
| 'istanbul ignore file', | |
| 'c8 ignore next' |
| import "canvas"; // jsdom will pick this up automatically (peer dep) | ||
| import 'canvas'; // jsdom will pick this up automatically (peer dep) | ||
| // Or, alternative: jest-canvas-mock if you only need basic APIs: | ||
| // import "jest-canvas-mock"; |
There was a problem hiding this comment.
This commented-out import statement still uses double quotes. To maintain consistency with the goal of this PR to standardize on single quotes, it should also be updated.
| // import "jest-canvas-mock"; | |
| // import 'jest-canvas-mock'; |
… push on failure, plus extended tests-and-lint-as-precommit-hook.md with setup steps, execution notes, and a quick verification hint
…t before the Playwright run, keeping e2e isolated. (package.json) Added test:core that chains format, lint, dependency checks, and unit/property tests for reuse in CI and locally. (package.json) Pointed the CI workflow’s test job at npm run test:core, allowing the GitHub Action to skip Playwright setup. (ci.yml)
1 participant
This pull request updates documentation to improve clarity, consistency, and code style in both the code coverage ignore guidelines and the Three.js test environment playbook. The main changes involve normalizing code examples to use single quotes, updating formatting to match modern JavaScript/TypeScript conventions, and clarifying instructions and examples for coverage ignores and test setups.
Code Coverage Ignore Guidelines (
docs/codeCoverageIgnoreGuidelines.md):Code style and formatting improvements:
Clarifications and minor text updates:
Three.js Test Environment Playbook (
docs/threeJs un test environments.md):Code style and formatting improvements:
Clarifications and minor text updates:
These changes make the documentation more consistent and easier to follow for both code style and testing best practices.
References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19]