chore: Remove retries for testcafe tests but increase timeout. Wait for lag in parallel #1380
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Size Change: +2.47 kB (+0.21%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
robbie-c
force-pushed
the
fix/increase-test-timeout
branch
from
ca68132 to
1e3655b
Compare
robbie-c
force-pushed
the
fix/increase-test-timeout
branch
from
e76e314 to
cb53922
Compare
robbie-c
force-pushed
the
fix/increase-test-timeout
branch
from
1472351 to
e6aab80
Compare
robbie-c
force-pushed
the
fix/increase-test-timeout
branch
from
8017b2a to
0252a45
Compare
src/entrypoints/recorder.ts
Outdated
| @@ -203,7 +203,7 @@ function _tryReadXHRBody({ | |||
| if (isObject(body)) { | |||
| try { | |||
| return JSON.stringify(body) | |||
| } catch (e) { | |||
| } catch (_) { | |||
There was a problem hiding this comment.
Feels like these should just be empty rather than have an underscore 🤔
There was a problem hiding this comment.
This is to workaround IE11 not supporting the optional catch binding syntax, i.e.
try {
// some stuff
} catch {
// other stuff
}
is not valid for IE11. There's a comment in this PR that links to some more details, see https://github.com/PostHog/posthog-js/pull/1380/files#diff-e2954b558f2aa82baff0e30964490d12942e0e251c1aa56c3294de6ec67b7cf5R12
There was a problem hiding this comment.
(I'm not sure if we can just rely on babel to handle this for us, if we can then we might just want to change all of these)
There was a problem hiding this comment.
Yeah looks like we can, so I'll change these
testcafe/check-testcafe-results.js
Outdated
There was a problem hiding this comment.
This the only thing I don't love... Feels like a hacky js script that we will not enjoy modifying and therefore won't improve upon later 🤔
All the hacky import stuff feels really nasty and it seems only to avoid making those assertion and loader helpers more pure. I appreciate this PR eats a lot of time so not going to block but this file is for sure going to be a headache in the future and cause people to turn away from improving things...
There was a problem hiding this comment.
Ah, sorry to hear that you don't like this. I thought it was much easier to read the test code with the assertions in the same file as tests, so I prioritised that over making the setup code readable, and these hacks were included to make this work.
Changes
Problem: previously, the test had a relatively short timeout that was sometimes less than ingestion lag. It also retried 3x, and had 3 tests where the ingestion lag ran in series. This meant that if the tests were bounded by ingestion lag, they would take ingestion lag x9 to finish.
This remove those whole-suite-level retry. It also pulls the query logic out of the test code and moves it to a later step, so instead of doing:
it now does
It's obviously still not going to work if there's 3 hours of ingestion lag, but should be more resilient to e.g. 5 minutes of lag.
Checklist