E2E tests: Dashboard - Edit Free Listings

[jorgemd24]

Changes proposed in this Pull Request:

Closes part of #2070

This PR aims to create the base for testing the dashboard and editing the free listings. To access the dashboard page, merchants must log in with their Google account, merchant centre, and so on; otherwise, they'll be redirected to the Getting Started page. Since the redirection occurs on the server and can't be easily mocked through requests, so I opted to simulate the onboarded state by changing the values returned from the database. This was addressed by introducing a filter, as seen in this commit: 68d76b3. This enables us to attach an action that emulates the onboarding state. Initially, I attempted to store options in the database similarly to how it's done for the conversion id:

google-listings-and-ads/tests/e2e/bin/test-data.php

Lines 19 to 36 in 551b385

	function register_routes() {
	register_rest_route(
	'wc/v3',
	'gla-test/conversion-id',
	[
	[
	'methods' => 'POST',
	'callback' => __NAMESPACE__ . '\set_conversion_id',
	'permission_callback' => __NAMESPACE__ . '\permissions',
	],
	[
	'methods' => 'DELETE',
	'callback' => __NAMESPACE__ . '\clear_conversion_id',
	'permission_callback' => __NAMESPACE__ . '\permissions',
	],
	],
	);
	}

However, this approach would cause issues, as it would lead to all tests being in the onboarded state, preventing us from testing scenarios like the getting started page.

This PR covers the testing of the following aspects:

• The dashboard page is rendered correctly.
• Tests the presence of both Free Listings and Edit buttons on the dashboard page.
• Verifying that clicking the Edit button triggers the display of a modal containing two options: "Continue to Edit" and "Don't Edit".
• Checking the checkbox for recommended shipping rates.
• Entering shipping times.
• Selecting tax rates.
• Testing the functionality of the "Save changes" button.- Asserting the requests generated during the process of saving changes.

Screenshots:

Detailed test instructions:

1. Follow these steps to set up the e2e env: https://github.com/woocommerce/google-listings-and-ads#e2e-testing
2. Run npm run test:e2e ./tests/e2e/specs/dashboard/edit-free-listings.test.js or npm run test:e2e to run all tests.
3. Check that all tests ✅

Additional details:

While working on this, I experienced the problem described here: #2043 (review). Specifically, the test-data plugin failed to load correctly, leading to 404 errors in certain tests or preventing the successful simulation of the onboarded state:

As a temporary solution, I had to stop the containers and restart them. Additionally, on occasions, I had to manually activate the plugin by visiting http://localhost:8889/wp-admin/plugins.php?plugin_status=all. We will need to look deeper into this possible issue.

While there are numerous other scenarios that could be tested, the primary objective of this PR is to create the base for the tests for the dashboard page.

Changelog entry

Dev - E2E - Dashboard - Edit Free Listings

[codecov]

Codecov Report

Merging #2080 (d0c9172) into develop (7812c76) will not change coverage.
Report is 3 commits behind head on develop.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff            @@
##             develop   #2080   +/-   ##
=========================================
  Coverage       58.3%   58.3%           
  Complexity      4118    4118           
=========================================
  Files            454     454           
  Lines          17683   17683           
=========================================
  Hits           10304   10304           
  Misses          7379    7379           

Flag	Coverage Δ
php-unit-tests	58.3% <ø> (ø)

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

[jorgemd24]

The JS linting is complaining about this:

/home/runner/work/google-listings-and-ads/google-listings-and-ads/tests/e2e/specs/dashboard/edit-free-listings.test.js
  56:31  error  Replace `·` with `⏎↹↹↹`  prettier/prettier
  72:28  error  Replace `·` with `⏎↹↹↹`  prettier/prettier

However, when I run npm run lint:js locally it is all good. Not sure if it is related to a Carriage Return and some tabs?

It seems related to this: https://stackoverflow.com/questions/67702186/prettier-ask-me-to-replace-with, I will have a look at it tomorrow.

[eason9487]

The JS linting is complaining about this:

/home/runner/work/google-listings-and-ads/google-listings-and-ads/tests/e2e/specs/dashboard/edit-free-listings.test.js
  56:31  error  Replace `·` with `⏎↹↹↹`  prettier/prettier
  72:28  error  Replace `·` with `⏎↹↹↹`  prettier/prettier

However, when I run npm run lint:js locally it is all good. Not sure if it is related to a Carriage Return and some tabs?

The prettier packages were updated in #2069. Rebasing onto the current develop should resolve the inconsistency.

[mikkamp]

This was addressed by introducing a filter, as seen in this commit: 68d76b3. This enables us to attach an action that emulates the onboarding state. Initially, I attempted to store options in the database similarly to how it's done for the conversion id
However, this approach would cause issues, as it would lead to all tests being in the onboarded state, preventing us from testing scenarios like the getting started page.

I have a question about this approach. For the conversion ID it's possible to both set and unset the conversion ID, which in the case of the GTag tests it's set through test.beforeAll and unset in test.afterAll so it will only be in that state for a certain set of tests. Can't we apply the same here and enable it only for a certain set of tests, since it uses an API request it's very quick to set/unset data in a specific state?
My reasoning why I prefer not to use the filter approach is because it will end up in the automatically generated hook document, but it's not a filter that should be used outside of E2E testing. Also as we build out the tests we are going to need many more "test" states (such as MC account connected, Ads account connected, etc...), so we'll have to add multiple filters which are intended for testing only.

[ianlin]

Thanks for adding the tests, especially the shared util classes so other tests can use them. I've tested it and the tests are working as expected. Just added a async await suggestion in the comment.

[mikkamp]

While working on this, I experienced the problem described here: #2043 (review). Specifically, the test-data plugin failed to load correctly, leading to 404 errors in certain tests or preventing the successful simulation of the onboarded state

Since you are able to reproduce this at least some of the time could you try to see if some tweaks would work? One such suggestion would be to map the plugin to a separate folder instead of having it directly in wp-content/plugins, so if we change:

		"wp-content/plugins/test-data.php": "./tests/e2e/bin/test-data.php"

		"wp-content/plugins/test-data/test-data.php": "./tests/e2e/bin/test-data.php"

Another suggestion would be to clear cache. As get_plugins caches the list of plugins and if it's missing from the list it outputs the message that the header is missing. Maybe running wp cache delete plugins plugins before activating the plugin.

[jorgemd24]

@mikkamp, thanks for your comment:

I have a question about this approach. For the conversion ID it's possible to both set and unset the conversion ID, which in the case of the GTag tests it's set through test.beforeAll and unset in test.afterAll so it will only be in that state for a certain set of tests. Can't we apply the same here and enable it only for a certain set of tests, since it uses an API request it's very quick to set/unset data in a specific state?

That was my first idea. However, the issue I encountered was that Playwright uses multiple workers to execute tests. This means that, for instance, the get-started.test.js test might run concurrently with edit-free-listings.test.js. Therefore, when edit-free-listings.test.js updates the database to simulate the onboarded merchant, it causes get-started.test.js to fail since the getting started page won't load instead it will load the dashboard or the product feed.

I think that if you make another test that needs the ADS_CONVERSION_ACTION option to be blank, you may encounter the problem because gtag-events.test.js will set it in the DB that is shared for all tests.

My reasoning why I prefer not to use the filter approach is that it will end up in the automatically generated hook document, but it's not a filter that should be used outside of E2E testing. Also as we build out the tests we are going to need many more "test" states (such as MC account connected, Ads account connected, etc...), so we'll have to add multiple filters which are intended for testing only.

I agree that currently, this filter is only used for E2E tests. However, I don't see needing multiple filters since the extensions mainly use REST endpoints, which can be easily simulated with Playwright. For example, when dealing with MC accounts and other accounts, we're using this approach: https://github.com/woocommerce/google-listings-and-ads/pull/2080/files#diff-f64b9525d898297dabb242cc89a61158db06de7159c4ec987ca62f6bb790e143R56-R74.

Since you are able to reproduce this at least some of the time could you try to see if some tweaks would work? One such suggestion would be to map the plugin to a separate folder instead of having it directly in wp-content/plugins, so if we change:

Thanks for all your suggestions! The issue is intermittent but I will try with those suggestions to see if it fixes it.

[mikkamp]

That was my first idea. However, the issue I encountered was that Playwright uses multiple workers to execute tests.

Thanks for the clarification, I forgot about that part and was originally contemplating turning that off like is done in Bookings when run for CI.
Do you think it will allow us to simplify tests if we don't run them in parallel? So we can set a state, run a series of tests and then reset the state? We use that same model for our unit tests as well.

Having a closer look now at the filter it seems you only do so when a query parameter is being set and not persistently, which seems like a workable solution and not too intrusive. I'm just wondering if that technique will eventually contribute to more code to handle certain states while supporting parallel tests, and whether just turning off parallel would simplify things.

[jorgemd24]

Thanks @ianlin for your time reviewing this PR. I have updated the below points in this PR:

• Adjusted the number of workers to 1 (d68f4d7) as it was decided in the dev call peeuvX-Te-p2#comment-1093.
  (cc: @mikkamp ).
• Removed filter (0664818 ) and add an endpoint to simulate the onboarded status (d3b3448 ).
• Added missing awaits (4b4f063)
• Fixed the JS Linting error (62f6270) (thanks @eason9487 )

Would you mind to take a look? Thanks!

[ianlin]

Thanks for the extra changes. I tested it again and everything passed as expected. Just left some non-blocking questions and suggestions in the comment below.

Note that during the review I also encountered this problem #2043 (review). I had to restart the container to make it work again. I tried to delete the plugins cache using wp cli in the container but still didn't work so I think those potential fixes @mikkamp suggested also require restarting the container so it makes it a bit tricky to verify if the fixes work. I'll keep trying it when developing other e2e tests.

[ianlin]

While the test is configured to run serial, I think it'd still be better to move this line to beforeAll() like dashboardPage.

[ianlin]

Typo: should be "Audience".

[ianlin]

❔We didn't mock this request /gla/mc/settings/sync and by running npm run test:e2e-dev tests/e2e/specs/dashboard/edit-free-listings.test.js I saw this request returned 500 server error with the message Please reconnect your Jetpack account.. Although the test still passed since we only check the response from gla/mc/settings API, do you think it'd be a good idea to mock /gla/mc/settings/sync API so it wouldn't respond the error?

[jorgemd24]

At first, I didn't mock the response because I believed there wasn't much to test. But now I realize we can actually verify the message in the snackbar. See here: 5a9c0e2

[jorgemd24]

Thanks @ianlin for your suggestions! I addressed all of them.

Note that during the review I also encountered this problem #2043 (review). I had to restart the container to make it work again. I tried to delete the plugins cache using wp cli in the container but still didn't work so I think those potential fixes @mikkamp suggested also require restarting the container so it makes it a bit tricky to verify if the fixes work. I'll keep trying it when developing other e2e tests.

Since yesterday I have been working with this tweak:

Another suggestion would be to clear cache. As get_plugins caches the list of plugins and if it's missing from the list it outputs the message that the header is missing. Maybe running wp cache delete plugins plugins before activating the plugin.

It looks like it's solving the problem. Once I confirm this, I'll implement the tweak.

[ianlin]

Thanks for the extra changes! I've tested again and it worked perfectly, LGTM.