Move user syncs to each respective adapter directory #688

zachbadgett · 2018-09-19T05:27:57Z

Continued progress on adapter isolation.

Code complete and unit tests updated, still need to build and run it.

zachbadgett · 2018-09-19T05:30:40Z

internal/testutil/usersync.go

+}
+
+// Assert common values match.
+func (u *usersyncTest) Assert(url, syncType string, vendor uint16, supportCORS bool) {


Maybe separate each value out into their own method like AssertFamilyName

zachbadgett · 2018-09-19T05:34:36Z

adapters/pubmatic/usersync.go

+	"github.com/prebid/prebid-server/usersync"
+)
+
+func NewPubmaticSyncer(cfg *config.Configuration) usersync.Usersyncer {


Another way to do this function is to have a signature like:

func NewPubmaticSyncer(externalURL string, adapter config.Adapter) usersync.Usersyncer

which would avoid the need for some adapters to do a lookup on the adapter map.

I'd also give a small edge to NewPubmaticSyncer(externalURL string, adapter config.Adapter), for the same reason you mentioned... but would accept a PR with either choice.

I think the main reason I kept it that way was in case there were other configuration additions. I say we keep it for now and change it later if it makes sense.

zachbadgett · 2018-09-19T05:39:14Z

internal/testutil/usersync.go

@@ -0,0 +1,44 @@
+package testutil


I don't like the idea of a "testutil" package...however, it's in the internal directory which is not exposed to the public api.

Thoughts?

I'm a fan of code sharing. In the vast majority of cases, utils are much better than duplicate code.

I'm not too worried about the package API, either, because this is an application and not a library. Our REST APIs are very important... but the package APIs are less so because we don't expect people to use this code by importing it. Any package structure within this project is purely for our own sanity.

That said: if you take a step back, do you think these utilities really improve things? Compare these code snippets (loosely based on the sovrn syncer).

Without utils (using testify):

info := syncer.GetUsersyncInfo("0", "") assert.Equal(t, "//ap.lijit.com/pixel?redir=external.com%2Fsetuid%3Fbidder%3Dsovrn%26gdpr%3D0%26gdpr_consent%3D%26uid%3D%24UID", info.URL) assert.Equal(t, "redirect", info.Type) assert.False(t, info.SupportCORS) assert.Equal(t, 13, syncer.GDPRVendorID())

With utils:

u := testutil.UsersyncTest(t, syncer, syncer.GetUsersyncInfo("0", "")) u.Assert( "//ap.lijit.com/pixel?redir=external.com%2Fsetuid%3Fbidder%3Dsovrn%26gdpr%3D0%26gdpr_consent%3D%26uid%3D%24UID", "redirect", 13, false, )

Advantages of no utils:

Fewer lines of code

Clearer to read

No need to implement the utils function

No need to find a good package location for the utils implementation

Advantages of utils:

When a field gets added to the usersyncer, we'd update u.Assert and nobody's tests would compile until every adapter is running the assertion

IMO it doesn't seem like the utils do enough to justify their existence... but let me know if I'm missing something.

I was mostly avoiding adding another dependency, didn't realize testify was already being used so I will update it.

One quick comment about your statement:

I'm not too worried about the package API, either, because this is an application and not a library.

Treating the components as libraries and stabilizing the API will allow the community to use the code here and contribute back improvements. Keeping it isolated as an application will cause users to fork and less likely to contribute anything back (I know I sound like a broken record at this point, however I really feel like this will improve the health of the project).

That's an interesting perspective... I wouldn't have thought this would make it hard for people to contribute to the project.

You're not a broken record at all. From your previous comments, I thought your main issue was that you or someone you knew was trying to add a Bidder and finding it annoying to change lots of different places in the code. I didn't realize you planned to import some packages and rely on them directly. Thanks for the perspective.

I do agree that the healthier an application architecture is, the more it resembles a bunch of libraries with a thin layer tying them all together... so whether or not we ultimately fold package APIs into our "v1" compatibility, it's definitely helpful to organize the code as if we plan to.

dbemiller · 2018-09-25T19:14:46Z

Hey @zachbadgett... this still looks like a work-in-progress to me, but let me know if I'm mistaken and you're waiting on a review for it. Or re-edit the title to remove the [WIP]

zachbadgett · 2018-09-26T15:58:11Z

@dbemiller this is done unless you have any other changes/comments.

dbemiller

yep yep, thanks!

Overall the structure is totally sensible. I do see a few issues too though.

dbemiller · 2018-09-26T14:38:18Z

usersync/usersyncers/syncer_test.go

-		}
+func TestNewSyncerMap(t *testing.T) {
+	cfg := &config.Configuration{
+		Adapters: map[string]config.Adapter{


Could you use the openrtb_ext.BidderMap populate this?

I have an overarching goal: when someone tries to contribute an adapter, make it "impossible" to do wrong. Whenever possible, that means relying on the compiler & types. When types aren't enough possible, rely on the automated tests & CI.

The main value of TestSyncers was to make sure that every Bidder in the project also had a Usersyncer. By defining the constants separately here, that's no longer enforced.

In an effort to isolate adapters entirely I was thinking of doing something like this: zachbadgett@57a580c#diff-25c7c8df3a616bd75098f16d158d0a0bR29

This could still achieve your goal of "impossible" to do wrong by doing a panic on Register.

The usersync/usersyncers/ would ultimately be removed.

Keep in mind the design decisions I made in that commit attempts to change other code as little as possible (especially in the adapters themselves).

yeah, that's fair.

I'm trying to make sure that the code is left in a good state after each PR. Someone might try to submit a Bidder between now and the time the next PR merges, or the next PR might never come.

We might also spend a while discussing tradeoffs. For init & Register in particular, I know we'll have at least some back and forth. I'm most definitely not an experienced Gopher... but I can think of some good language-agnostic reasons to avoid that structure, and the benefits that I can think of don't outweigh the costs. As an avid Go supporter, I'm curious to learn about how you weigh them :).

That's a long-winded way of saying: if the future changes don't pan out, I'd rather not lose the value provided by this test.

dbemiller · 2018-09-26T15:40:50Z

adapters/syncer_test.go

+
+func TestSyncers(t *testing.T) {
+	syncers := make(map[openrtb_ext.BidderName]usersync.Usersyncer)
+	for _, bidderName := range map[string]openrtb_ext.BidderName{} {


Dead test? This ranges over an empty map.

This may be related to the TestSyncerMap() comment I made elsewhere. Looks like it may have gotten moved, changed a bit, and forgotten about?

Not forgotten about, see my comment above. The map is ultimately moved to the adapters package, however importing the static map from where it lives now would cause a circular dependency.

aha, yeah that makes sense. I wasn't sure which one of the two tests you wanted to be the "real" one.

Can you either move the static map into this package, or just delete this test? It doesn't make much sense to have a test without assertions.

dbemiller · 2018-09-26T15:42:48Z

adapters/syncer_test.go

+func TestVendorIDUniqueness(t *testing.T) {
+	syncers := make(map[openrtb_ext.BidderName]usersync.Usersyncer)
+	idMap := make(map[uint16]openrtb_ext.BidderName, 0)
+	for name, syncer := range syncers {


Same thing here... range over an empty map.

Given the circular dependency issue: could you either move this test back into the usersyncers package for now, or else move the static map into this package to avoid the circular dependency?

Until the Register/panic changes are in, this test does have some value... and I have a few good reasons not to go that route, so I'd rather treat this PR as an isolated change set while we discuss.

Sure thing!

dbemiller · 2018-09-26T17:28:24Z

endpoints/openrtb2/openrtb2_go111_test.go

@@ -0,0 +1,23 @@
+// +build go1.11


Can you split this Go 1.11 fix off into a separate PR? It's a valuable change... but commits do get squashed on merge, and it's unrelated.

Small note: I'd prefer something like message_go1.11 rather than message_alt. We already know that this bug has been fixed in Go 1.11.1 (see golang/go#27275), so... it's a pretty specific fix.

Would also be nice to lin this issue somewhere in the file so we remember why the workaround exists.

I was thinking the same thing for the message name. I went with message_alt because it will apply to go1.11+, now that I know it was an actual bug and it's been fix it makes a lot more sense to do message_go1.11.

hhhjort · 2018-10-29T14:02:20Z

Looks like some of David's comments haven't been addressed yet? Are there still changes in the works, or is the current state what you think the final state should be?

zachbadgett · 2018-10-29T17:15:59Z

This is the final state, I fixed what he commented on. I'll resolve the conflicts today.

dbemiller · 2018-10-30T16:13:39Z

oh! Sorry. I didn't know you were waiting on me here.

Open issues I see are these:

Goals are:

Don't waste cycles running a test with no assertions
Have a test which fails if someone contributes a Bidder but forgets their Usersyncer.

Also: merge conflicts.

Add internal/testutil package

zachbadgett · 2018-10-30T17:13:18Z

Heh...I moved the tests but didn't commit the reverting of them. My mistake! Should be fixed now.

dbemiller · 2018-10-30T17:15:47Z

adapters/syncer_test.go

@@ -0,0 +1 @@
+package adapters


Got an empty file here too... not sure if I only just noticed this now, or it became empty in those last changes.

That's where those usersync tests were...and will be again eventually, not a big deal to delete for now.

* Move user syncs to each respective adapter directory Add internal/testutil package * Update usersync tests to use testify's assert * Update syncer tests * Add ix and rhythmone usersyncs * Remove syncer_test.go

zachbadgett force-pushed the usersync-isolation branch from 63d48d9 to fb011e2 Compare September 19, 2018 05:28

zachbadgett commented Sep 19, 2018

View reviewed changes

zachbadgett force-pushed the usersync-isolation branch 2 times, most recently from 5a18782 to 544500b Compare September 19, 2018 05:46

zachbadgett force-pushed the usersync-isolation branch 3 times, most recently from eacd396 to 1862d05 Compare September 25, 2018 22:22

zachbadgett changed the title ~~[WIP] Move user syncs to each respective adapter directory~~ Move user syncs to each respective adapter directory Sep 25, 2018

dbemiller suggested changes Sep 26, 2018

View reviewed changes

dbemiller requested a review from hhhjort September 26, 2018 17:45

dbemiller assigned dbemiller and hhhjort Sep 26, 2018

zachbadgett force-pushed the usersync-isolation branch 3 times, most recently from 0020e2d to 93754f7 Compare October 1, 2018 18:42

zachbadgett added 4 commits October 30, 2018 10:54

Move user syncs to each respective adapter directory

fbfd103

Add internal/testutil package

Update usersync tests to use testify's assert

c0cfc63

Update syncer tests

ff8f4a9

Add ix and rhythmone usersyncs

70b3d9a

zachbadgett force-pushed the usersync-isolation branch from 93754f7 to 70b3d9a Compare October 30, 2018 17:11

dbemiller reviewed Oct 30, 2018

View reviewed changes

Remove syncer_test.go

0386137

dbemiller approved these changes Oct 30, 2018

View reviewed changes

hhhjort approved these changes Oct 31, 2018

View reviewed changes

hhhjort merged commit 72b524e into prebid:master Oct 31, 2018

zachbadgett deleted the usersync-isolation branch October 31, 2018 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move user syncs to each respective adapter directory #688

Move user syncs to each respective adapter directory #688

zachbadgett commented Sep 19, 2018

zachbadgett Sep 19, 2018 •

edited

Loading

zachbadgett Sep 19, 2018 •

edited

Loading

dbemiller Sep 19, 2018 •

edited

Loading

zachbadgett Sep 25, 2018 •

edited

Loading

zachbadgett Sep 19, 2018

dbemiller Sep 19, 2018 •

edited

Loading

zachbadgett Sep 25, 2018 •

edited

Loading

dbemiller Sep 26, 2018

dbemiller commented Sep 25, 2018 •

edited

Loading

zachbadgett commented Sep 26, 2018

dbemiller left a comment

dbemiller Sep 26, 2018

zachbadgett Sep 26, 2018

zachbadgett Sep 26, 2018

dbemiller Sep 28, 2018

dbemiller Sep 26, 2018

zachbadgett Sep 26, 2018

dbemiller Sep 28, 2018 •

edited

Loading

dbemiller Sep 26, 2018

dbemiller Sep 28, 2018

zachbadgett Sep 28, 2018

dbemiller Sep 26, 2018

zachbadgett Sep 26, 2018

hhhjort commented Oct 29, 2018

zachbadgett commented Oct 29, 2018

dbemiller commented Oct 30, 2018 •

edited

Loading

zachbadgett commented Oct 30, 2018

dbemiller Oct 30, 2018

zachbadgett Oct 30, 2018

Move user syncs to each respective adapter directory #688

Move user syncs to each respective adapter directory #688

Conversation

zachbadgett commented Sep 19, 2018

zachbadgett Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

zachbadgett Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

dbemiller Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

zachbadgett Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

zachbadgett Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller commented Sep 25, 2018 • edited Loading

zachbadgett commented Sep 26, 2018

dbemiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbemiller Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort commented Oct 29, 2018

zachbadgett commented Oct 29, 2018

dbemiller commented Oct 30, 2018 • edited Loading

zachbadgett commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachbadgett Sep 19, 2018 •

edited

Loading

zachbadgett Sep 19, 2018 •

edited

Loading

dbemiller Sep 19, 2018 •

edited

Loading

zachbadgett Sep 25, 2018 •

edited

Loading

dbemiller Sep 19, 2018 •

edited

Loading

zachbadgett Sep 25, 2018 •

edited

Loading

dbemiller commented Sep 25, 2018 •

edited

Loading

dbemiller Sep 28, 2018 •

edited

Loading

dbemiller commented Oct 30, 2018 •

edited

Loading