ZeopTapId: Fix broken unit tests

[robertrmartinez]

Type of change

• Bugfix

Description of change

The zeoTap Id system was relying on storageManager to be directly writing cookies in all browsers. This caused circleCi to fail.

So, the real thing all UNIT tests should be doing is mocking their storage manager.

This PR mocks the storage utilities necessary in the zeotap spec file.

[smenzer]

good to know for future id modules that are doing similar things. does it matter if it's local storage or cookies? i see audigent calls localstorage directly (https://github.com/prebid/Prebid.js/pull/5524/files#diff-d0da60273284688dcaace1e87e2ca3d7R1131) and quantcast is calling setcookie directly too (https://github.com/prebid/Prebid.js/pull/5727/files#diff-58dc06f8386f9012950e1e3e2f67e699R5). @robertrmartinez can you take a look at those two as well to make sure they're fixed if needed?

[robertrmartinez]

It should not matter, as long as they are using the storageManager properly, we can stub it and test the correct scenarios. local storage or cookies.

The only downside is not catching something browser specific that changes with cookies. For example, with the CHROME 85 release they did away with the setting of cookies with sameSite none when secure flag not set.

So something we may want to talk about, a suite of tests just for storageManager maybe.

[smenzer]

agreed that storageManager should probably have its own tests. the quantcast and audigent tests were passing, so maybe there isn't an issue with them?

[robertrmartinez]

Yeah, depending on how tests are written, it is not 100% that tests will fail when directly accessing local storage or cookies.

But we just should not be doing it in my opinion. So I believe a migration of all spec files which do so, needs to be done.

Wether that be writing some storage mock that will work with all module unit tests, or going to each individual one.

Would like to hear from some others @jaiminpanchal27 @jsnellbaker @harpere @msm0504.

What do you guys think is the best solution?

[jsnellbaker]

So I think this approach is okay for now and may potentially be something we'd want to replicate/adopt for others doing something similar.

We could try to look into making a unified stub (similar I think to what was attempted with adloader.js, though I don't know if that was properly utilized). But we'd have to allow for some flexibility for when there are unique tests/params that have to be replaced (like what was done here, where we're supposed to provide a very specific return value when a given function was called).

Curious for other thoughts/opinions.

[robertrmartinez]

Yeah,

I can try to come up with a universal stub idea when I get some extra time.

Then once we agree on it, we can migrate existing ones.

But it is a little challenging because of the many functions on storageManagers, not including the hooks into gdprEnforcement and stuff. GVL IDs, module names...

Prob will have to spec the newStorageManager to return a stripped down version or something.

But, for now, importing their own storage object like done in this PR, and mocking the functions they need should suffice.

The downside to this is we are not testing browsers capability to set cookies or local storage. So I am curious as to if we want a separate suite of tests which make sure storageManager itself can set cookies in these circleCi test browsers.

Would at least shed light to changes between browser versions like what we saw with Chrome 84->85. And we can address and let people know if they fail.

If everyone is okay with this fix for now, I say we merge it and release 4.8.0 today.

And we can come up with the long term solution later?

[smenzer]

Plan sounds good to me. Can you please create an Issue so we don’t forget?

[robertrmartinez]

Good idea @smenzer

#5762