Skip to content
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

core(trace-processing): add backport for pubads #14700

Merged
merged 4 commits into from
Jan 24, 2023

Conversation

adamraine
Copy link
Member

Looks like #14693 broke pubads which was causing the errors in our e2e tests. This is a hacky workaround to fix pubads on main.

Our smokes didn't catch this because we don't check any pubads audits that use trace processing.

We could also just revert #14693, doesn't seem super important.

@adamraine adamraine requested a review from a team as a code owner January 20, 2023 20:44
@adamraine adamraine requested review from connorjclark and removed request for a team January 20, 2023 20:44
@brendankenny
Copy link
Member

Good call on getting it into smoke tests. Can we update pubads? We are still (in theory - #14375) going to get an official release there)

@adamraine
Copy link
Member Author

I guess we could, pubads is already using a dev version of lighthouse:
https://github.com/googleads/publisher-ads-lighthouse-plugin/blob/f05bf852f0e9e772a14ab408d55b0852672daac6/package.json#L25

@adamraine
Copy link
Member Author

adamraine commented Jan 23, 2023

So there are a lot of changes we would need to make for pubads to work with the latest dev release of LH (not just this trace processor stuff). I would prefer to keep the backport and do those changes when 10.0 is released officially.

@connorjclark
Copy link
Collaborator

connorjclark commented Jan 23, 2023

Are we to ask them to do an official release where the pinned version for the LH dep is one of our nightlies? And then post-10.0, we can change that to 10.0 proper?

@brendankenny
Copy link
Member

I would prefer to keep the backport and do those changes when 10.0 is released officially.

That seems fine, maybe just add a note to #14375 to get rid of the workaround?

This probably warrants a discussion about testing for plugin compatibility when we're explicitly working on a major version release with breaking changes. For 11.0, for instance, should we just disable the pubads tests and reenable when there's a release supporting 11? Downside is we wouldn't catch unexpected regressions, but I'm not sure how likely that is given that we should have pretty good coverage for the artifacts they need, etc.

@adamraine adamraine merged commit 1826c75 into main Jan 24, 2023
@adamraine adamraine deleted the fix-pubads-trace-processor branch January 24, 2023 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants