Root out legacy upload APIs from the test-utils reporter #606
Conversation
|
|
/release-pr |
packages/playwright/src/reporter.ts
Outdated
| packageName, | ||
| packageVersion, | ||
| }); | ||
|
|
||
| if (accessToken) { | ||
| initLaunchDarklyFromAccessToken(accessToken); |
There was a problem hiding this comment.
This is async but we can't even await things here, this is race'y. But what should be done about it - should getFeatureFlagValue await the pending identification?
There was a problem hiding this comment.
Hmm... initLaunchDarklyFromAccessToken is async because it fetches auth info.
The reporter base class (in test-utils) already awaits auth info in order to initialize the Grafana logger. Should we move LD initialization in there?
should
getFeatureFlagValueawait the pending identification?
It already does :) That's how I designed it.
There was a problem hiding this comment.
Hmm... initLaunchDarklyFromAccessToken is async because it fetches auth info.
The same is true about MixPanel's initialization - both should get the same treatment here.
The reporter base class (in test-utils) already awaits auth info in order to initialize the Grafana logger. Should we move LD initialization in there?
I'll look into this.
It already does :) That's how I designed it.
It waits on the LD's initialization - but is this the same as the user's identification? 🤔
There was a problem hiding this comment.
Perhaps I misremembered what it's doing 😆 You're right. I think waiting some small amount (before falling back to the default user profiles) seems reasonable?
packages/playwright/src/reporter.ts
Outdated
| packageName, | ||
| packageVersion, | ||
| }); | ||
|
|
||
| if (accessToken) { | ||
| initLaunchDarklyFromAccessToken(accessToken); |
There was a problem hiding this comment.
Hmm... initLaunchDarklyFromAccessToken is async because it fetches auth info.
The reporter base class (in test-utils) already awaits auth info in order to initialize the Grafana logger. Should we move LD initialization in there?
should
getFeatureFlagValueawait the pending identification?
It already does :) That's how I designed it.
|
RE: the changes to (Not that it should necessarily impact this PR, just an FYI) |
Ye, I reverted the LD-related changes from this PR as it wasn't completely obvious how it should work. I'll lean on PRO-797 to improve this |
I noticed that we removed the file related to multipart upload from the files that we ported to test-utils. Luckily, we also forgot to initialize LD... so those code paths were never hit here (cc @callingmedic911 ).
This sparked a further refactor to get rid of the legacy uploading code from the test-utils and to use the one that we have in
/shared.