Skip to content
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

[API PULL] Adding more tests for Notifications #2482

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

puntope
Copy link
Contributor

@puntope puntope commented Jul 28, 2024

Changes proposed in this Pull Request:

This PR adds more tests for Notification Schedules in API Pull

Screenshots:

Screenshot 2024-07-28 at 17 18 55

Detailed test instructions:

  1. See the test passing locally in your machine npm run test:e2e notifications
  2. See the test passing in GH Actions

@puntope puntope requested a review from a team July 28, 2024 13:37
@puntope puntope marked this pull request as ready for review July 28, 2024 13:37
@github-actions github-actions bot added the changelog: dev Developer-facing only change. label Jul 28, 2024
Copy link

codecov bot commented Jul 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.5%. Comparing base (c3f4679) to head (a1a0ff7).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2482      +/-   ##
============================================
- Coverage       64.5%   63.5%    -1.0%     
============================================
  Files            795     322     -473     
  Lines          22844    5043   -17801     
  Branches        1220    1220              
============================================
- Hits           14739    3204   -11535     
+ Misses          7938    1672    -6266     
  Partials         167     167              
Flag Coverage Δ
js-unit-tests 63.5% <ø> (ø)
php-unit-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 473 files with indirect coverage changes

Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for adding e2e tests for notifications. All the tests have passed in my local, LGTM.

Just a note that it initially failed in my local with the following error:

✘  6 [chromium] › notifications/notifications-schedule.test.js:67:6 › Notifications Schedule › When Notifications are ready. Notifications are scheduled (35.2s)

      at ../utils/product-editor.js:263

  261 | 			return expect(
  262 | 				page.locator( '.product_data_tabs li.active' )
> 263 | 			).toHaveCount( 1 );
      | 			  ^
  264 | 		},
  265 |
  266 | 		async clickPluginTab() {

    at Object.waitForInteractionReady (/Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/utils/product-editor.js:263:6)
    at Object.gotoAddProductPage (/Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/utils/product-editor.js:170:15)
    at /Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/specs/notifications/notifications-schedule.test.js:69:3

After checking a bit and found it's because my local wp-env docker was built previously, and it had the new product block editor feature enabled. In your tests it tested on the classic product editor so the error occurred. Maybe we could make it a follow up by disabling the new product block editor feature when testing on classic product editor. We currently turn on the product block feature when testing it, but not the other way around.

@puntope
Copy link
Contributor Author

puntope commented Jul 29, 2024

Thanks for adding e2e tests for notifications. All the tests have passed in my local, LGTM.

Just a note that it initially failed in my local with the following error:

✘  6 [chromium] › notifications/notifications-schedule.test.js:67:6 › Notifications Schedule › When Notifications are ready. Notifications are scheduled (35.2s)

      at ../utils/product-editor.js:263

  261 | 			return expect(
  262 | 				page.locator( '.product_data_tabs li.active' )
> 263 | 			).toHaveCount( 1 );
      | 			  ^
  264 | 		},
  265 |
  266 | 		async clickPluginTab() {

    at Object.waitForInteractionReady (/Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/utils/product-editor.js:263:6)
    at Object.gotoAddProductPage (/Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/utils/product-editor.js:170:15)
    at /Users/ianlin/dev/automattic/google-listings-and-ads/tests/e2e/specs/notifications/notifications-schedule.test.js:69:3

After checking a bit and found it's because my local wp-env docker was built previously, and it had the new product block editor feature enabled. In your tests it tested on the classic product editor so the error occurred. Maybe we could make it a follow up by disabling the new product block editor feature when testing on classic product editor. We currently turn on the product block feature when testing it, but not the other way around.

INteresting @ianlin Yes. I will create an issue for this.

@puntope puntope merged commit 4164bc2 into develop Jul 29, 2024
10 checks passed
@puntope puntope deleted the dev/e2e-notifications branch July 29, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: dev Developer-facing only change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants