-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Flaky Jetpack E2E: revise Advertising: Promote
spec to use the first post and to not create posts unnecessarily.
#81563
Flaky Jetpack E2E: revise Advertising: Promote
spec to use the first post and to not create posts unnecessarily.
#81563
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
- implement new method to see if there are posts on the site. test/e2e/specs/tools/advertising__promote.ts - stop creating new post unnecessarily
f93ca7d
to
19b9206
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀 left a couple of non-blocking suggestions. Feel free to merge! 👍🏻
* @param param1 Keyed object parameter. | ||
* @param {SitePostState} param1.state State of the published post. | ||
*/ | ||
async siteHasPost( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): We can also check for the expected response structure & API-specific errors.
if ( response.hasOwnProperty( 'error' ) ) {
throw new Error(
`${ ( response as ErrorResponse ).error }: ${ ( response as ErrorResponse ).message }`
);
}
if (!response || !response.counts || !response.counts.all) {
throw new Error('Unexpected response structure');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I played around with this a bit, and it appears this endpoint doesn't ever return an error - it only returns a 404 (if anything in the path is wrong, for example site ID) or the actual values.
params | ||
); | ||
|
||
return response.counts.all[ state ] !== 0 ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): We can simplify this to return response.counts.all[state] !== 0
since it already returns a boolean value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, what an embarrassingly unnecessary ternary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've all been there! 😄
- remove unnecessary ternary packages/calypso-e2e/src/types/rest-api-client.types.ts - add new type to type check the response from PostCounts endpoint.
Related to: #80730, #81550.
Proposed Changes
This PR further revises the Advertising: Promote spec to stop the creation of unnecessary posts.
Furthermore, this PR also changes the selection method of the post from title-based to row number based,
Testing Instructions
Ensure the following build configurations are passing:
This particular PR survived 50 rounds of repeated testing:
Pre-merge Checklist