Add E2E tests for Enhanced Conversions data collection during WPForms submissions#13162
Add E2E tests for Enhanced Conversions data collection during WPForms submissions#13162JakePT wants to merge 5 commits into
Conversation
🎭 Playwright reports for bc00f45: |
eugene-manuilov
left a comment
There was a problem hiding this comment.
Thanks, @JakePT. This is a good start. Left a number of comments for you. Please, take a look.
| env: | ||
| # Update this date suffix (YYYYMMDD) whenever the bundled plugins in the playwright-wp Dockerfile change. | ||
| PLAYWRIGHT_WP_DATE_SUFFIX: '20260715' | ||
| PLAYWRIGHT_WP_DATE_SUFFIX: '20260722' |
There was a problem hiding this comment.
We don't need to change the image version if the Dockerfile hasn't been changed. Since the wpforms plugin has already been added to the image, no changes to the Dockerfile are needed. So, please revert the tag changes in this PR.
| test.describe.configure( { mode: 'serial' } ); | ||
|
|
||
| test.beforeEach( async ( { wp } ) => { | ||
| await wp.activatePlugin( 'wpforms-lite/wpforms.php' ); |
There was a problem hiding this comment.
This should be done using withPlugins.
There was a problem hiding this comment.
@eugene-manuilov It looks like withPlugins only works with plugins in google-site-kit-test-plugins/. I see a few options:
- Update
withPluginsto fall back to plugins outside that directory. - Split
withPluginsintowithPluginsandwithTestPluginsfor plugins ingoogle-site-kit-test-plugins/. - Stick with
wp.activatePlugin.
What do you think?
There was a problem hiding this comment.
Update withPlugins to fall back to plugins outside that directory.
Yes, let's update withPlugins to allow other plugins to be enabled as well. Perhaps we can consider plugins with / as 3rd plugins and not prepend google-site-kit-test-plugins/?
There was a problem hiding this comment.
@eugene-manuilov I've updated withPlugins to not prepend the test plugins directory if the plugin file name includes a /.
| 'WPForms Enhanced Conversions', | ||
| { annotation: [ plugins, anonymousUser ] }, | ||
| () => { | ||
| test.describe.configure( { mode: 'serial' } ); |
There was a problem hiding this comment.
All tests must run in parallel.
There was a problem hiding this comment.
@eugene-manuilov I've removed this setting.
| async verifyEmailFormEvent(): Promise< void > { | ||
| await this.fillAndSubmit( [ SAMPLE_FORM_DATA.email ] ); | ||
|
|
||
| const payload = await waitForGTagEvent( this.page, 'submit_lead_form' ); | ||
|
|
||
| expect( payload ).toMatchObject( { | ||
| event_source: 'site-kit', | ||
| googlesitekit_event_provider: 'wpforms', | ||
| user_data: { | ||
| email: SAMPLE_FORM_DATA.email.normalizedValue, | ||
| }, | ||
| } ); | ||
| expect( payload.googlesitekit_form_id ).toMatch( /^\d+$/ ); | ||
| } |
There was a problem hiding this comment.
These verify* methods should be actual Playwright tests. Please, move them into the spec file.
There was a problem hiding this comment.
@eugene-manuilov My thinking here was to make potential future specs for other form plugins as minimal as possible by making these shared methods available, and this is the same pattern used by email-reporting-page.ts and pdf-generation-page.ts, which also have verify* methods that call expect() in the page object. What do you think?
| * @param eventName The gtag event name. | ||
| * @return The event payload, or null when the event has not fired. | ||
| */ | ||
| export function getGTagEvent( |
There was a problem hiding this comment.
getGTagEvent and waitForGTagEvent functions should be part of the FormsPage page object class.
| add_filter( | ||
| 'rest_endpoints', | ||
| function ( $endpoints ) { | ||
| $route = '/' . Google\Site_Kit\Core\REST_API\REST_Routes::REST_ROOT . '/core/site/data/conversion-tracking'; | ||
|
|
||
| foreach ( array_keys( $endpoints[ $route ] ) as $key ) { | ||
| if ( 'namespace' === $key ) { | ||
| continue; | ||
| } | ||
|
|
||
| $endpoints[ $route ][ $key ]['permission_callback'] = '__return_true'; | ||
| } | ||
|
|
||
| return $endpoints; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Hm... I can't say I like this. Instead of changing our REST route permissions, we should use a different approach to send information from tests to e2e plugins. For example, we can do it similarly to how we send other dynamic information. This will require you to set a special cookie for tests that need enhanced conversion enabled, and then use a filter hook to override the setting if the cookie is set in the request. See how we do it to set user or module information.
There was a problem hiding this comment.
@eugene-manuilov I've changed the approach to add a withConversionTracking annotation that sets a cookie that an MU plugin uses to filter the setting value, following the pattern used by other annotations like withFeatureFlags.
| if ( defined( 'WP_CLI' ) && WP_CLI ) { | ||
| WP_CLI::add_command( 'site-kit-e2e create-wpforms-fixtures', 'google_site_kit_e2e_create_wpforms_fixtures' ); | ||
| } |
There was a problem hiding this comment.
Let's probably move this code that generates test forms and pages to a new file wpforms.php in the tests/playwright/bin folder and then execute it using the wp eval-file command? Something like this:
cat wpforms.php | wp eval-file -
In this case, this code will be close to the script that actually generates data and won't mess with the plugin we use for testing.
There was a problem hiding this comment.
@eugene-manuilov I've moved this code to tests/playwright/bin/create-wpforms-fixtures.php, which is run during backup with wp eval-file.
|
@eugene-manuilov I've addressed all of your feedback except for this one where I've left a comment. Playwright / WordPress 5.2.21 is failing but I'll probably need your help understanding what's going on there. It looks like WPForms isn't active so maybe something is wrong with how I've used |
Summary
Addresses issue:
Relevant technical choices
wp ability runto create forms but the oldest version of WPForms Lite that supports this command does not support WordPress 5.2 so I registered a custom CLI command intests/playwright/docker/wordpress/plugins/enhanced-conversions.phpand called that to do it with PHP.I started with the
implement-issueskill with Codex andgpt-5.6-soland it went pretty well. It caught the issue with the ability command in the IB but it needed a fair amount of handholding to implement a different approach. Because the original IB had the form creation in the CLI it tried to do a different approach entirely within the CLI and needed to be guided towards a cleaner solution. The other results still needed a lot of work but that was not really anything to do with the skill, and more with how the model implemented the IB.PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist