feat(track): Introducing easier event tracking #250
Conversation
|
|
||
| decisions.add(decision); | ||
| } | ||
| ArrayList<Decision> decisions = new ArrayList<Decision>(); |
There was a problem hiding this comment.
This looks like it's only being used below once. We can skip instantiating this list if we:
Snapshot snapshot = new Snapshot.Builder()
- .setDecisions(decisions)
+ .setDecisions(Collections.emptyList())
.setEvents(Collections.singletonList((conversionEvent)))
.build();| @@ -104,28 +104,13 @@ public LogEvent createImpressionEvent(@Nonnull ProjectConfig projectConfig, | |||
| } | |||
|
|
|||
| public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig, | |||
| @Nonnull Map<Experiment, Variation> experimentVariationMap, | |||
There was a problem hiding this comment.
Not sure if we want to avoid breaking API changes in this PR, but if we do, we may want to overload this method rather than removing argument since this is technically public even though we probably don't encourage direct.
Edit: disregard. I was going off of this method+class being public, but it lives in internal package, which is good-enough warning of being a non-stable API in my opinion.
| testParams + " and payload \"{}\""); | ||
|
|
||
| optimizely.track(eventType.getKey(), genericUserId, attributes); | ||
| verify(mockEventHandler).dispatchEvent(eq(logEventToDispatch)); |
There was a problem hiding this comment.
minor: since we are mocking the event factory (for better or worse), maybe consider sameInstance() instead of eq() for a little more stringency. also can consider using verifyNoMoreInteractions(mockEventFactory) if that's what we expect. if you decide to incorporate, could probably do in a helper method on this test class, as this isn't the only test it could be applied to.
|
@aliabbasrizvi can you resolve the conflicts please |
|
@mikeng13 will do that when I respond to feedback. |
|
All conflicts resolved and made changes as per discussion with @loganlinn and @mjc1283 |
|
I am running this PR against tests from Matt's PR to test EET in compat suite: https://github.com/optimizely/fullstack-sdk-compatibility-suite/pull/199/files I created a branch that enables it for the Java SDK: Test Run: |
| .setDecisions(decisions) | ||
| .setEvents(Collections.singletonList((conversionEvent))) | ||
| .build(); | ||
| .setEvents(Collections.singletonList((conversionEvent))) |
There was a problem hiding this comment.
extra set of parenthesis
Pull Request Test Coverage Report for Build 835
💛 - Coveralls |
|
Tests passed here: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/97428057 Will merge change. |
Summary
This change updates track to always send an event without doing experiment/variation lookup.
Test plan