Infrastructure/11740 playwright email reporting#12330
Infrastructure/11740 playwright email reporting#12330eugene-manuilov wants to merge 41 commits intodevelopfrom
Conversation
…notations and a new mu-plugin.
…setup' into infrastructure/11740-playwright-email-reporting.
…40-playwright-email-reporting.
📚 Storybook for 446fc7b: 📦 Build files for 446fc7b:
🎭 Playwright reports for 446fc7b: |
|
Size Change: 0 B Total Size: 2.31 MB ℹ️ View Unchanged
|
…it consistent and repeatable across all tests.
…40-playwright-email-reporting.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // Ensure every notice after the first notice has a margin | ||
| // so the notices don't render with no gap. | ||
| // |
There was a problem hiding this comment.
Stylelint blamed this and insisted it be removed. 🤷♂️
…40-playwright-email-reporting.
…40-playwright-email-reporting.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @eugene-manuilov, this looks great overall, just a few things to address. Also note there are a few other comments from GitHub + annotations from JS Lint to review.
| - analyticsadmin.googleapis.com | ||
| - analyticsdata.googleapis.com | ||
| - searchconsole.googleapis.com | ||
| - oauth2.googleapis.com | ||
| - tagmanager.googleapis.com | ||
| - adsense.googleapis.com | ||
| - pagespeedonline.googleapis.com | ||
| - subscribewithgoogle.googleapis.com | ||
| - www.googleapis.com | ||
| - storage.googleapis.com |
There was a problem hiding this comment.
We should include googleads here too since it's used by PAX, if only to prevent requests to the real services. Also, people and siteverification are missing.
There was a problem hiding this comment.
Site verification uses www.googleapis.com, which is already added. See here: https://developers.google.com/site-verification/v1/webResource/get
Which one Google Ads API uses?
There was a problem hiding this comment.
Oh interesting, ok, thanks 👍
Ads uses googleads.googleapis.com
See https://developers.google.com/google-ads/api/rest/design/overview#resource-oriented-design
| update_user_option( | ||
| get_current_user_id(), | ||
| 'googlesitekit_site_verified_meta', | ||
| 'verified' | ||
| ); | ||
|
|
||
| $settings = get_option( Search_Console_Settings::OPTION ); | ||
| $settings['propertyID'] = 'http://localhost:9002'; | ||
| update_option( Search_Console_Settings::OPTION, $settings ); |
There was a problem hiding this comment.
Can we not use filters for these instead? Otherwise we're running these updates on every request. I think this is how we did it before, no?
There was a problem hiding this comment.
Slightly updated it to update the search console settings only once. Re site verification, not sure why but it works only when i do it like that. If I switch to the filter hook, it stops working.
There was a problem hiding this comment.
Ah ok. I see we do it like that in our current E2E utility but it happens in a REST endpoint we call rather than on every request.
| 'should deliver a weekly email report', | ||
| { | ||
| annotation: [ | ||
| withPlugins( 'mailpit.php', 'email-reporting.php' ), |
There was a problem hiding this comment.
I think mailpit should be part of the always-on infra as an mu-plugin. No reason for it not to be, right?
There was a problem hiding this comment.
I don't think so. If we don't use it, it shouldn't be activated in my opinion.
There was a problem hiding this comment.
If we don't use it and an email is sent for whatever reason I believe it can result in a fatal error (at least in some situations, it has in the past). The service will be running anyways, so I don't see why we wouldn't have it as part of the infrastructure. We shouldn't need to remember to turn it on; we should only be concerned with assertions as to whether an email was sent or not.
There was a problem hiding this comment.
Ok, made it to be mu-plugin
| * | ||
| * @param array $http_client_config Guzzle HTTP client configuration array. | ||
| */ | ||
| $http_client_config = apply_filters( 'googlesitekit_http_client_config', $http_client_config ); |
There was a problem hiding this comment.
I'd rather not add a filter just for use in E2E, but I'm not sure there is another way. Alternatively, we could conditionally only apply it in tests/e2e – WDYT?
There was a problem hiding this comment.
Unfortunately, i can't find another way how to do it.
There was a problem hiding this comment.
After thinking about this some more, I think we can overload the access token value which we're already controlling with a filter in tests to use a plain test-access-token. We can essentially append/embed additional metadata (or just pass json in this field, encoded if necessary) and then we can pass whatever we want through to the service and it will be contained in tests. This would be extensible to other values or configuration we want to send through as well. Yes, it gets encrypted (this is necessary) but it will be decrypted before being set in the authorization header. I think this should work since we should also never need to use a fixture for an unauthenticated response. Let me know what you think 👍
There was a problem hiding this comment.
Nice idea, updated.
There was a problem hiding this comment.
Actually, it turned out if we do it this way, auto-authorization stops working. I had to revert it to the previous approach with the filter. I think we can create a ticket to address this challenge separately.
...ht/docker/fixtures/data/email-reporting/weekly-report-data/searchconsole.googleapis.com.json
Show resolved
Hide resolved
| try { | ||
| startServers(); | ||
| } catch ( err ) { | ||
| global.console.error( | ||
| 'Failed to start HTTPS server:', | ||
| err instanceof Error ? err.message : String( err ) | ||
| ); | ||
| global.console.log( 'Continuing with HTTP only.' ); | ||
| } |
There was a problem hiding this comment.
startServers starts both http and https but there's no guarantee that http started successfully in the catch here, no?
There was a problem hiding this comment.
Yes, but that is correct. I don't really think we will actually need the http version though, because all APIs have https URLs.
There was a problem hiding this comment.
My point is that the messages here imply that http will always start but we can't say for sure. It would be better to just say that there was an error starting the servers and not claim that http will still work or that the error was from starting https specifically.
There was a problem hiding this comment.
Removed extra logging
…40-playwright-email-reporting.
…40-playwright-email-reporting.
…40-playwright-email-reporting.
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @eugene-manuilov this one is almost there, back to you for a few final things. See also my comment/replies on previous threads which aren't listed below.
| add_filter( 'googlesitekit_email_reporting_batch_id', fn() => 'test-batch-id' ); | ||
|
|
||
| do_action_ref_array( Email_Reporting_Scheduler::ACTION_INITIATOR, array( $frequency ) ); | ||
| do_action_ref_array( Email_Reporting_Scheduler::ACTION_WORKER, array( 'test-batch-id', $frequency, time() ) ); |
There was a problem hiding this comment.
I think we can do without the filter on the batch ID too, we just need to query/capture the batch it creates when the initiator runs.
There was a problem hiding this comment.
Good point. Updated.
| add_filter( | ||
| 'wp_mail_from_name', | ||
| function () { | ||
| return 'Site Kit E2E Tests'; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Let's not override the from name – it shouldn't be necessary and it could be worth allowing for an assertion on the name.
| * | ||
| * @param string $batch_id The batch ID. | ||
| */ | ||
| $batch_id = apply_filters( 'googlesitekit_email_reporting_batch_id', wp_generate_uuid4() ); |
There was a problem hiding this comment.
See my other comment on this one, as I think we can do without it as well.
| }, | ||
| { | ||
| "keys": [ | ||
| "+44 (20) 738 4500" |
There was a problem hiding this comment.
This looks like a real number?
| ) | ||
| .join( '\n' ); | ||
| const uniqueErrors: string[] = []; | ||
| errors.forEach( ( e ) => { |
There was a problem hiding this comment.
A number of lint warnings related to this as well
…40-playwright-email-reporting.
…40-playwright-email-reporting.
3 participants
Summary
Addresses issue:
Relevant technical choices
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