-
Notifications
You must be signed in to change notification settings - Fork 506
chore: Add Playwright tests to CI to check out stability for the UI #567
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
chore: Add Playwright tests to CI to check out stability for the UI #567
Conversation
|
@rogaldh is attempting to deploy a commit to the Solana Foundation Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 28b1511 in 2 minutes and 57 seconds. Click for details.
- Reviewed
2124lines of code in35files - Skipped
1files when reviewing. - Skipped posting
15draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yaml:55
- Draft comment:
Ensure that installing Playwright dependencies via pnpx is intentional; consider using 'pnpm exec' if consistent. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Both pnpx and pnpm exec are valid ways to run binaries. While consistency is nice, this is more of a style preference than a functional issue. The current command will work correctly. The comment is asking for confirmation of intention rather than pointing out a real problem. The suggestion could improve consistency in the codebase, and inconsistent package manager usage might confuse future maintainers. While consistency is good, this is a minor stylistic preference that doesn't affect functionality. The comment starts with "Ensure that..." which violates our rules about not asking for confirmation. Delete the comment as it's asking for confirmation of intention and suggesting a style change that isn't functionally necessary.
2. .storybook/main.ts:31
- Draft comment:
Remove or document the commented-out storybookTest plugin configuration if no longer needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Commented-out code can be a code smell and cleaning it up is generally good practice. However, this appears to be part of a larger work-in-progress refactoring of the _viteFinal configuration, with multiple commented sections. The author likely left these comments intentionally while working through different approaches. The comment doesn't add much value in this context. The commented code could be technical debt that needs addressing. The unused import also suggests this needs cleanup. While technical debt is important, this appears to be active development work rather than abandoned code, given the multiple variations being tried. The author is likely aware of the comments they just added. The comment should be deleted as it's pointing out something the author is actively working with and is likely temporary during development.
3. app/address/[address]/layout.tsx:217
- Draft comment:
Header component now wraps AccountHeader; verify that behaviours from removed MetaplexNFTHeader are preserved. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that behaviors are preserved, which is against the rules. It does not provide a specific suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. tailwind.config.ts:30
- Draft comment:
Good use of getScreenDim to adjust breakpoints—but ensure the 1px shift aligns with design requirements. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. vite.config.mts:21
- Draft comment:
Alias configuration looks thorough; consider consolidating duplicate aliases for clarity. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. vitest.workspace.ts:18
- Draft comment:
The workspace uses a Storybook test plugin with nodePolyfills; double-check that the globals configuration meets all test cases. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
7. app/components/account/TokenAccountSection.tsx:212
- Draft comment:
The 'Bridged Asset Contract' row uses tokenInfo.extensions.bridgeContract in the href, but it should likely reference tokenInfo.extensions.assetContract instead. Please verify and fix the URL. - Reason this comment was not posted:
Comment was on unchanged code.
8. .storybook/main.ts:31
- Draft comment:
There is commented-out code for the storybookTest plugin. If no longer needed, consider removing it to clean up the configuration. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
9. app/tx/[signature]/page-client.tsx:107
- Draft comment:
The auto-refresh effect uses the status dependency to update zeroConfirmationRetries. Consider reviewing the dependency array of the useEffect hook to ensure stable behavior if status toggles frequently. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
10. vite.config.mts:21
- Draft comment:
Aliases for '@/app', '@app', etc. are repeated. Consider consolidating them to avoid duplication. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. test-setup.ts:13
- Draft comment:
Overriding Uint8Array[Symbol.hasInstance] with Buffer.isBuffer is a known hack; ensure it’s well documented and reviewed for potential edge cases. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
12. .github/workflows/ci.yaml:10
- Draft comment:
Typo detected in the inline comment on line 10. It currently reads 'cash folder for pnpm_store, nextjs and playwright later on'. Please update 'cash' to 'cache' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. app/components/account/AccountHeader.tsx:168
- Draft comment:
Typo: The branding for 'Coingecko' should be properly capitalized as 'CoinGecko' to match the official styling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While technically correct about the official branding, this is a very minor UI detail. Our rules specifically say not to comment on pure frontend UI changes and to assume the author made UI choices correctly. This feels more like a style preference than a critical issue requiring a code change. The branding consistency could be important for professionalism and legal compliance. Incorrect capitalization could potentially confuse users about whether they're dealing with the official service. While brand consistency has merit, this is still fundamentally a UI text change. The link still works correctly and users can clearly understand what service is being referenced. Our rules explicitly state to avoid pure UI feedback. This comment should be removed as it deals with UI text styling, which our rules explicitly say to ignore.
14. app/components/account/TokenAccountSection.tsx:212
- Draft comment:
Typo: In the asset contract row, the anchor tag's href incorrectly referencestokenInfo.extensions.bridgeContractinstead oftokenInfo.extensions.assetContract. Please update the href to usetokenInfo.extensions.assetContractso that it links to the correct URL. - Reason this comment was not posted:
Comment was on unchanged code.
15. app/components/account/token-extensions/TokenExtensionsStatusRow.tsx:12
- Draft comment:
Typo detected: The word 'compoent' should be corrected to 'component' in the comment on line 12. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_cFTgJ18xqSXPDEnj
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Facing issue at the CI side: |
|
I removed the script for playwright from CI, but we can run it locally. Executing it at CI will be the next step. It seems reasonable to implement this in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 5891871 in 1 minute and 42 seconds. Click for details.
- Reviewed
70lines of code in3files - Skipped
1files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:17
- Draft comment:
Changed 'test:sb' script to non-watch mode; consider adding an alternate script for watch mode if interactive debugging is needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The change deliberately removes watch mode from the storybook tests. This appears intentional since there's already a pattern of having separate watch/non-watch scripts (test vs test:ci). The comment suggests adding complexity that may not be needed - if watch mode was needed, the author would likely have kept it. The comment is more of a suggestion than pointing out a clear issue. The author may have removed watch mode for a specific reason, like improving CI performance. Having both watch and non-watch modes could be useful for different development workflows. While having both modes could be nice, this is more of an optional enhancement than a clear issue. The default should be to trust the author's judgment on their testing workflow needs. Delete the comment. It's making a suggestion about a deliberate change without strong evidence that an additional watch mode script is actually needed.
2. package.json:49
- Draft comment:
The pinned experimental version for @storybook/addon-vitest should be documented to clarify its necessity and potential caveats. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking for documentation on a specific version of a dependency, which is not allowed according to the rules. It doesn't provide a specific code suggestion or ask for a test to be written.
3. package.json:103
- Draft comment:
Using '^8.6.12' for @storybook/experimental-nextjs-vite allows patch updates; verify this change is intentional and compatible with your setup. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is related to a dependency version change. It asks the author to verify the change, which is against the rules. The comment does not provide a specific suggestion or highlight a specific issue with the code.
4. vitest.workspace.ts:32
- Draft comment:
Switching browser configuration from a single 'name' to an 'instances' array improves configurability; ensure this new syntax is fully supported by the Playwright provider. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that the new syntax is fully supported by the Playwright provider. This falls under the category of asking the author to ensure something is tested or verified, which is against the rules.
Workflow ID: wflow_EDakPFPkrsxuxrKb
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Can you squash the git history? |
c8ee98c to
7f0abbf
Compare
@ngundotra done |
7f0abbf to
e6b5e14
Compare
That is fixed at the new version of the runner |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 394df59 in 53 seconds. Click for details.
- Reviewed
161lines of code in3files - Skipped
1files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/ci.yaml:72
- Draft comment:
Verify that switching the cache key from PW_CACHE_AFFIX to PNPM_CACHE_AFFIX in the Additional Cache step is intentional. Mixing cache keys for multiple directories could lead to unintended cache invalidation. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/ci.yaml:99
- Draft comment:
Confirm that 'continue-on-error: true' for the Storybook-Smoke-Tests job is desired, as it will allow the workflow to pass even if UI smoke tests fail. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. .storybook/vitest.setup.ts:13
- Draft comment:
The async beforeAll with a 100ms delay is a useful workaround for the race condition. Please ensure that this delay is sufficient across all environments. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. vitest.workspace.ts:45
- Draft comment:
Test timeout increased to 15000ms and additional settings (hookTimeout, retry, sequence) were added. Confirm these values optimize stability without unnecessarily slowing tests. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. vitest.workspace.ts:38
- Draft comment:
The browser configuration now conditionally disables Firefox in CI and sets sequential test execution. Ensure that running tests sequentially meets performance expectations in non-CI environments. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_Deju7E5IoohkItaY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
This PR covers the changes been made by #731 |
Description
Add separate job to check for smoke-tests to ensure UI is not broken
Type of change
Screenshots
Testing
pnpm test:ci && pnpm test:sbRelated Issues
HOO-235
Checklist
Important
Add Playwright tests for Storybook to CI with caching and race condition fixes.
Storybook-Smoke-Testsjob inci.yamlto run Playwright tests for Storybook.ci.yaml.STORYBOOK_DISABLE_TELEMETRYinci.yaml.beforeAllinvitest.setup.tsto fix race condition.vitest.workspace.tsto include node polyfills and adjust test settings (timeouts, retries).This description was created by
for 394df59. You can customize this summary. It will automatically update as commits are pushed.