GoogleChromeLabs / third-party-capital

A library that provides best practices for loading popular third-parties
Apache License 2.0
116 stars 8 forks source link

Add PHP test coverage for the actual third parties implemented #66

Closed felixarntz closed 1 month ago

felixarntz commented 2 months ago

So far, the PHP codebase only has unit test coverage for all its foundation. While this is great, what we're missing is a set of integration tests that ensure that with the JSON file structure the output of the HTML, scripts etc. generated matches the expected.

This is particularly crucial in this repository since we're dealing with two implementations of the same thing (one in JS, one in PHP), but they always need to match, or be kept in sync.

@flashdesignory Looking at some of the PRs that landed over the recent months, I have a hunch the PHP codebase will have some bugs now because it seems some of the JSON file values were changed in technically backward-incompatible ways - which would be fine for JS because you've updated it accordingly, but I think the corresponding PHP code is now outdated.

It's a bit of a painful situation to have to keep these in sync, but having test coverage should ensure we don't miss anything in that regard in the future. Separately, I wonder if there's a better way for us to stay on track of such changes. FWIW with the PHP side we're mostly following on what you do on the JS side, so it may be good to simply flag any PR where the JSON is changed to us (@adamsilverstein and me). Let me know if you have other ideas how we can improve this. In any case, PHP test coverage for this should help.

felixarntz commented 2 months ago

cc @housseindjirdeh

flashdesignory commented 2 months ago

@felixarntz - totally understood that this is currently painful 😦

I don't anticipate any additional changes to the schema or the json output in the near future though. The js side has been tested and implemented in two major JavaScript frameworks (which resulted in the changes in the last few months). Now I do feel that we have a dataset that should work for most implementations.

I assume without the changes, you'd probably would have run into the same issues we solved.

Happy to discuss testing with you all to avoid this scenario to happen again.