-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(ui-tests): scaffolds basic test #388
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
yarn.lock
shouldn't need to change, can you rebase or whatever?
Also Vercel lint is failing.
package.json
Outdated
@@ -56,6 +58,7 @@ | |||
"@vitejs/plugin-react": "^2.2.0", | |||
"@walletconnect/types": "2.11.0", | |||
"autoprefixer": "^10.4.14", | |||
"dotenv": "16.3.1", |
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.
Why was this added? I see the require('dotenv').config();
line was removed below
"dotenv": "16.3.1", |
I had merge conflicts. Removed |
Vercel is failing due to `` src/components/settings/PrivacySettings/index.tsx(44,10): error TS2741: Property 'children' is missing in type '{ title: string; subtitle: string; }' but required in type 'IProps'.08:56:06.184 | error Command failed with exit code 2.
|
@devceline can you fix this PR? |
@chris13524 I want to take a look at this. I could handle the tests and increase the coverage with the remaining tests cc @arein |
@enesozturk thank you. For the scope of this PR the only thing that's blocking is the failing Vercel deployment see:
Once this is fixed I think we can merge and then you can take over in follow up PRs |
* feat(ui-tests): scaffolds basic test * feat: tests * refactor: update breakpoints, handle breakpoint for explorer columns * Fix/force watch subscriptions (#393) * chore: add unregister functionality * chore: add unregister to facade * chore: add unregister others * chore: run prettier * chore(config): ensure Sentry captures `console.error` calls (#400) * chore: remove console capture (#410) * feat(ui-tests): scaffolds basic test * feat: tests * refactor: handle notification modal on ci, refactor tests * refactor: remove privacy settings comp * chore: update selectors * chore: revert apps max width --------- Co-authored-by: Derek <[email protected]> Co-authored-by: Celine Sarafa <[email protected]> Co-authored-by: Ben Kremer <[email protected]>
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.
LGTM other than some suggestions for future iterations
@@ -96,7 +97,7 @@ export const Modals = () => { | |||
|
|||
{shouldShowPWAModal && <PwaModal />} | |||
|
|||
{isNotificationPwaModalOpen && <NotificationPwaModal />} | |||
{!isCI && isNotificationPwaModalOpen && <NotificationPwaModal />} |
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.
I think we can avoid this flag by granting the notification
permission. Also we'll need to test these states anyway so this will have to change.
|
||
import { BASE_URL } from '../constants' | ||
|
||
export class ModalPage { |
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.
Rename to InboxPage
?
await this.page.getByText('Subscribed to', { exact: false }).isVisible() | ||
} | ||
|
||
async unsubscribe(nth: number) { |
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.
This API will flake in future revisions because number is non-deterministic. Better to select by name.
Basic tests covering subscribe/unsubscribe.
Description
Please include a brief summary of the change.
Type of change
Please put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Fixes/Resolves (Optional)
Please list any issues that are fixed by this PR in the format
#<issue number>
. If it fixes multiple issues, please put each issue in a separate line. If this Pull Request does not fix any issues, please delete this section.Examples/Screenshots (Optional)
Please attach a screenshot or screen recording of features/fixes you've implemented to verify your changes. Please also note any relevant details for your configuration. If your changes do not affect the UI, please delete this section.
Use this table template to show examples of your changes:
Checklist:
Additional Information (Optional)
Please include any additional information that may be useful for the reviewer.