-
Notifications
You must be signed in to change notification settings - Fork 308
Fix flaky E2E test in specs/modules/analytics/setup-gcp-no-account-no-tag.test.js
.
#10108
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
Comments
I wasn't able to find a clear reason these specific endpoints are failing while others are't, un-assigning myself so as not to burn cycles. |
Noting that recent failures are failing with the following error:
For example:
|
Thanks @techanvil – the fact that this is failing with a non-JSON response is important so I think we should be careful to not simply intercept the request and be done with it but first understand why this is. It's worth noting that we mock this endpoint on the server side in the "has account" side in
I believe the no-account variant enables this once the account is "created" so perhaps there is a timing issue. Even still, I wouldn't expect this to fail with invalid JSON but rather a different error. I.e. if it's invoking a request to the external API, it should return an "Invalid Credentials" error. I wonder if this may be due to the request being canceled by a navigation, in which case it might be happening in a scenario where it shouldn't be. |
Thanks @aaemnnosttv, agreed, it's worth digging in more deeply here. The With regard to the "with account" variants, it's worth noting a couple of things:
Taking the above into consideration, I think setting up the client-side Having tried using the existing It's admittedly hard to fully test this because the issue itself is not reproducible on demand, however if this provides effective we could consider replacing the existing I've updated the IB accordingly, interested to see what you think. |
Thanks @techanvil – thinking about this a bit more, as far as I recall cancelling results in a different error: Can you see what the response is when this is error is triggered? I think this would be useful to see the underlying value causing the error to confirm the cause. |
Hi @aaemnnosttv, as mentioned this is tricky to reproduce due to its sporadic nature. I'm trying to reproduce it by rerunning the test, however, in the meantime I do have a debug-enabled log for the similarly failing test in Full test output
Test output highlightsFirst request to
Second request to
As can be seen above, the second request to It does suggest the request is simply being interrupted for some reason; given the results I've had with the debounced |
Update: I've now got a log for this issue's test, which effectively exhibits the same result i.e. there's no response logged for the Full test output
Log from the `GET ...container-destinations` to test failure
|
Thanks @techanvil – I wonder if the error causes the response to not be logged by our infra as well? Two last things to consider here I think:
A bit puzzling why this one request only is failing like this, so I'd prefer to avoid ignoring it without understanding the root cause. |
Thanks @aaemnnosttv, I'm happy to look further into this but before doing so, should clarify that in fact it's not just this one request - I've seen the Given it's not confined to this one request, do you still think it's worth spending more time digging in here? Here's an example successful run, to illustrate where the second POST to Test output
|
I suppose this issue (#) is specific to the GCP scenario, which I don't think is worth putting substantial time into, however I believe these are happening in other scenarios too, no? I would like to know what the cause is but better to spend time on tests that are more valuable than this one. If it is happening on others, then let's dig into it a bit more but on those issues. If we find a common solution, we can apply it here (if it isn't solved at the same time). If not, we can address this one with a simpler solution. |
@aaemnnosttv, yup, we're seeing the same sort of error for a test in @hussain-t are you happy to dig into the cause of the JSON error on that issue, with reference to the history on this one? Or, if you prefer I can pick it up to continue the investigation from where it's got to here. Thinking about a potential simpler solution - if the error is indeed a result of a navigation while requests are still in flight for the Google tag settings sync, maybe we can simply use Anyhow, let's cross that bridge when we come back to it. Maybe the JSON error investigation will provide a new direction. |
Hey @aaemnnosttv, I've investigated the JSON error and have written up my findings here: #10109 (comment) In summary, it doesn't appear to be an issue with the response being returned from the backend, and is likely an edge case in how the interrupted requests are handled when navigating away from the page. I've included the same fix for this issue's IB, including adding a timeout to the |
Hey @aaemnnosttv, as per this comment, #10683 (comment), I've restored the proposal for a debounced version of |
Feature Description
We still have a number of E2E tests failing with some regularity in our CI test runs. The task of fixing them has been split into four issues in order to make it easier to investigate and fix. See #10109, #10110, #10111.
setting up the Analytics module using GCP auth with no existing account and no existing tag › displays account creation form when user has no Analytics account
Example: https://github.com/google/site-kit-wp/actions/runs/12874436085/job/35893852219#step:11:249
Details
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
createWaitForFetchRequests()
to support the ability to debounce requests, such that that resets a timeout for resolving its returned promise if a new request comes in before the specified debounce delay.specs/modules/analytics/setup-gcp-no-account-no-tag.test.js
:timeout
option towaitForSelector( '.googlesitekit-subtle-notification' )
with a value of10000
(10 seconds), in order to accommodate occasional spikes in the runner's load.createWaitForFetchRequests()
utility to wait for requests to complete before deactivating plugins in the test file'safterEach()
handler.setup-gcp-no-account-no-tag.js
E2E test failure. #10571.Test Coverage
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: