From 453b2b545ba394fe2352e58b8da197ab763101f3 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 13 Jun 2024 11:18:28 +0100 Subject: [PATCH 1/8] fix: notification dates edge case (#25148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Due to manual date comparisons, there were some weirdness with the dates displayed (such as a notification yesterday, but not 24 hours ago). This fix (using date built-ins) ensures correct notification dates. NOTE - need to port this on other platforms. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25148?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- ui/helpers/utils/notification.util.ts | 48 ++++++++-- ui/helpers/utils/notification.utils.test.ts | 97 +++++++++++++++++---- 2 files changed, 123 insertions(+), 22 deletions(-) diff --git a/ui/helpers/utils/notification.util.ts b/ui/helpers/utils/notification.util.ts index 31be80d4d8de..598936820a01 100644 --- a/ui/helpers/utils/notification.util.ts +++ b/ui/helpers/utils/notification.util.ts @@ -27,6 +27,41 @@ import { decimalToHex, } from '../../../shared/modules/conversion.utils'; +/** + * Checks if 2 date objects are on the same day + * + * @param currentDate + * @param dateToCheck + * @returns boolean if dates are same day. + */ +const isSameDay = (currentDate: Date, dateToCheck: Date) => + currentDate.getFullYear() === dateToCheck.getFullYear() && + currentDate.getMonth() === dateToCheck.getMonth() && + currentDate.getDate() === dateToCheck.getDate(); + +/** + * Checks if a date is "yesterday" from the current date + * + * @param currentDate + * @param dateToCheck + * @returns boolean if dates were "yesterday" + */ +const isYesterday = (currentDate: Date, dateToCheck: Date) => { + const yesterday = new Date(currentDate); + yesterday.setDate(currentDate.getDate() - 1); + return isSameDay(yesterday, dateToCheck); +}; + +/** + * Checks if 2 date objects are in the same year. + * + * @param currentDate + * @param dateToCheck + * @returns boolean if dates were in same year + */ +const isSameYear = (currentDate: Date, dateToCheck: Date) => + currentDate.getFullYear() === dateToCheck.getFullYear(); + /** * Formats a given date into different formats based on how much time has elapsed since that date. * @@ -34,11 +69,10 @@ import { * @returns The formatted date. */ export function formatMenuItemDate(date: Date) { - const elapsed = Math.abs(Date.now() - date.getTime()); - const diffDays = elapsed / (1000 * 60 * 60 * 24); + const currentDate = new Date(); - // E.g. Yesterday - if (diffDays < 1) { + // E.g. 12:21 + if (isSameDay(currentDate, date)) { return new Intl.DateTimeFormat('en', { hour: 'numeric', minute: 'numeric', @@ -46,8 +80,8 @@ export function formatMenuItemDate(date: Date) { }).format(date); } - // E.g. 12:21 - if (Math.floor(diffDays) === 1) { + // E.g. Yesterday + if (isYesterday(currentDate, date)) { return new Intl.RelativeTimeFormat('en', { numeric: 'auto' }).format( -1, 'day', @@ -55,7 +89,7 @@ export function formatMenuItemDate(date: Date) { } // E.g. 21 Oct - if (diffDays > 1 && diffDays < 365) { + if (isSameYear(currentDate, date)) { return new Intl.DateTimeFormat('en', { month: 'short', day: 'numeric', diff --git a/ui/helpers/utils/notification.utils.test.ts b/ui/helpers/utils/notification.utils.test.ts index cb37ffcbaaa0..e018b4076d22 100644 --- a/ui/helpers/utils/notification.utils.test.ts +++ b/ui/helpers/utils/notification.utils.test.ts @@ -6,31 +6,98 @@ import { } from './notification.util'; describe('formatMenuItemDate', () => { + beforeAll(() => { + jest.useFakeTimers(); + jest.setSystemTime(new Date('2024-06-07T09:40:00Z')); + }); + + afterAll(() => { + jest.useRealTimers(); + }); + it('should format date as time if the date is today', () => { - const date = new Date(); - const result = formatMenuItemDate(date); - expect(result).toMatch(/^\d{2}:\d{2}$/u); + const assertToday = (modifyDate?: (d: Date) => void) => { + const testDate = new Date(); + modifyDate?.(testDate); + expect(formatMenuItemDate(testDate)).toMatch(/^\d{2}:\d{2}$/u); + }; + + // assert current date + assertToday(); + + // assert 1 hour ago + assertToday((testDate) => { + testDate.setHours(testDate.getHours() - 1); + return testDate; + }); }); it('should format date as "yesterday" if the date was yesterday', () => { - const date = new Date(); - date.setDate(date.getDate() - 1); - const result = formatMenuItemDate(date); - expect(result).toBe('yesterday'); + const assertYesterday = (modifyDate: (d: Date) => void) => { + const testDate = new Date(); + modifyDate(testDate); + expect(formatMenuItemDate(testDate)).toBe('yesterday'); + }; + + // assert exactly 1 day ago + assertYesterday((testDate) => { + testDate.setDate(testDate.getDate() - 1); + }); + + // assert almost a day ago, but was still yesterday + // E.g. if Today way 09:40AM, but date to test was 23 hours ago (yesterday at 10:40AM), we still want to to show yesterday + assertYesterday((testDate) => { + testDate.setDate(testDate.getDate() - 1); + testDate.setHours(testDate.getHours() + 1); + }); }); it('should format date as "DD Mon" if the date is this year but not today or yesterday', () => { - const date = new Date(); - date.setMonth(date.getMonth() - 1); - const result = formatMenuItemDate(date); - expect(result).toMatch(/^\w{3} \d{1,2}$/u); + const assertMonthsAgo = (modifyDate: (d: Date) => Date | void) => { + let testDate = new Date(); + testDate = modifyDate(testDate) ?? testDate; + expect(formatMenuItemDate(testDate)).toMatch(/^\w{3} \d{1,2}$/u); + }; + + // assert exactly 1 month ago + assertMonthsAgo((testDate) => { + testDate.setMonth(testDate.getMonth() - 1); + }); + + // assert 2 months ago + assertMonthsAgo((testDate) => { + testDate.setMonth(testDate.getMonth() - 2); + }); + + // assert almost a month ago (where it is a new month, but not 30 days) + assertMonthsAgo(() => { + // jest mock date is set in july, so we will test with month may + return new Date('2024-05-20T09:40:00Z'); + }); }); it('should format date as "Mon DD, YYYY" if the date is not this year', () => { - const date = new Date(); - date.setFullYear(date.getFullYear() - 1); - const result = formatMenuItemDate(date); - expect(result).toMatch(/^\w{3} \d{1,2}, \d{4}$/u); + const assertYearsAgo = (modifyDate: (d: Date) => Date | void) => { + let testDate = new Date(); + testDate = modifyDate(testDate) ?? testDate; + expect(formatMenuItemDate(testDate)).toMatch(/^\w{3} \d{1,2}, \d{4}$/u); + }; + + // assert exactly 1 year ago + assertYearsAgo((testDate) => { + testDate.setFullYear(testDate.getFullYear() - 1); + }); + + // assert 2 years ago + assertYearsAgo((testDate) => { + testDate.setFullYear(testDate.getFullYear() - 2); + }); + + // assert almost a year ago (where it is a new year, but not 365 days ago) + assertYearsAgo(() => { + // jest mock date is set in 2024, so we will test with year 2023 + return new Date('2023-11-20T09:40:00Z'); + }); }); }); From 0dff71e7ad2934c68b55dbc63c3ed61843c40c63 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Thu, 13 Jun 2024 12:42:48 +0200 Subject: [PATCH 2/8] fix: flaky test `ENS domain resolves to a correct address` (#25248) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the flaky test `ENS domain resolves to a correct address` The error is: ``` [driver] Called 'clickElement' with arguments [".address-list-item"] [driver] Called 'findElement' with arguments [{"css":".ens-input__selected-input__title","text":"test.eth"}] Failure on testcase: 'ENS domain resolves to a correct address', for more information see the artifacts tab in CI TimeoutError: Waiting for element to be located By(xpath, .//*[contains(concat(' ', normalize-space(./@class), ' '), ' ens-input__selected-input__title ')][(contains(string(.), 'test.eth') or contains(string(.), 'test.eth'))])` ``` The problem is that we are clicking the address-list-item button, and nothing happens afterwards. Then we try to find the next element but is not there. If we look into the address-list-item button element we can see how it has nested elements inside, which will render the address and the ENS domain. Clicking on a "container" element with other elements inside might not work as we expect, since the inside elements might not be fully updated before clicking with unknown effects (in this case, the ENS and address resolution) . To fix this, we are doing 2 things: - waiting for both the ENS and the address to be fully rendered (before we were just waiting for the ENS domain to be loaded) - clicking on a more specific element inside the container -> we now click into the inner element for the domain ENS See Box as the container button, and nested elements with address and ENS domain. ![Screenshot from 2024-06-12 10-49-23](https://github.com/MetaMask/metamask-extension/assets/54408225/bef54bcf-9f67-46f3-9155-b877fb9c844f) - ci failure example: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/86997/workflows/200911a8-50a6-42f2-b56a-b6f7afc8fc1e/jobs/3178850/artifacts [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25248?quickstart=1) ## **Related issues** Fixes: https://github.com/MetaMask/metamask-extension/issues/24652 ## **Manual testing steps** 1. Check ci 2. Run test multiple times locally with different builds `yarn test:e2e:single test/e2e/tests/transaction/ens.spec.js --browser=chrome --leave-running --retryUntilFailure --retries=10` ## **Screenshots/Recordings** Ci failure screenshot: notice how, after clicking the ENS address button, we don't see the asset below, this means that the click didn't have any effect. ![image](https://github.com/MetaMask/metamask-extension/assets/54408225/c62f66e5-a194-4d70-917b-5677cdb13f87) Expected: after clicking the EN address button, we should see the asset below ![Screenshot from 2024-06-12 10-04-33](https://github.com/MetaMask/metamask-extension/assets/54408225/9478f881-edeb-4a32-9e95-ce715526e72b) ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. - [ ] --- test/e2e/tests/transaction/ens.spec.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/test/e2e/tests/transaction/ens.spec.js b/test/e2e/tests/transaction/ens.spec.js index 9497fe3ef60c..db399fef5a93 100644 --- a/test/e2e/tests/transaction/ens.spec.js +++ b/test/e2e/tests/transaction/ens.spec.js @@ -8,6 +8,11 @@ const FixtureBuilder = require('../../fixture-builder'); describe('ENS', function () { const sampleAddress = '1111111111111111111111111111111111111111'; + + // Having 2 versions of the address is a bug(#25286) + const shortSampleAddress = '0x1111...1111'; + const shortSampleAddresV2 = '0x11111...11111'; + const sampleEnsDomain = 'test.eth'; const infuraUrl = 'https://mainnet.infura.io/v3/00000000000000000000000000000000'; @@ -95,7 +100,15 @@ describe('ENS', function () { css: '[data-testid="multichain-send-page__recipient__item__title"]', }); - await driver.clickElement('.multichain-send-page__recipient__item'); + await driver.waitForSelector({ + text: shortSampleAddress, + css: '.multichain-send-page__recipient__item__subtitle', + }); + + await driver.clickElement({ + text: sampleEnsDomain, + css: '[data-testid="multichain-send-page__recipient__item__title"]', + }); await driver.findElement({ css: '.ens-input__selected-input__title', @@ -103,7 +116,7 @@ describe('ENS', function () { }); await driver.findElement({ - text: '0x11111...11111', + text: shortSampleAddresV2, }); }, ); From 933e09d9654ec33fac6a4f0ce7ad2c35a61b7822 Mon Sep 17 00:00:00 2001 From: seaona <54408225+seaona@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:49:19 +0200 Subject: [PATCH 3/8] fix: flaky test `Send NFT should not be able to send ERC1155 NFT with invalid amount` (#25289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** This PR fixes the flaky test `Send NFT should not be able to send ERC1155 NFT with invalid amount`. The error is `ElementNotInteractableError: Element