Refactor OptimizelyTest to have less reliance on Mockito. #308
Conversation
Pull Request Test Coverage Report for Build 1081
💛 - Coveralls |
Pull Request Test Coverage Report for Build 1093
💛 - Coveralls |
| event.getKey(), | ||
| visitor.getVisitorId(), | ||
| visitor.getAttributes().stream() | ||
| .filter(attribute -> !attribute.getKey().startsWith("$")) |
There was a problem hiding this comment.
nit. Would recommend changing to startsWith("$opt_") or ProjectConfig. RESERVED_ATTRIBUTE_PREFIX
| private String visitorId; | ||
| private Map<String, ?> attributes; | ||
|
|
||
| public CanonicalEvent(String experimentKey, String variationKey, String eventName, String visitorId, Map<String, ?> attributes) { |
There was a problem hiding this comment.
Shouldn't these be experimentId and variationId?
| @Override | ||
| public String toString() { | ||
| return new StringJoiner(", ", CanonicalEvent.class.getSimpleName() + "[", "]") | ||
| .add("experimentKey='" + experimentKey + "'") |
There was a problem hiding this comment.
experimentId= and variationId=
| assertNull(actualVariation); | ||
| } | ||
|
|
||
| /** | ||
| * Verify that {@link Optimizely#activate(String, String)} handles the case where an unknown experiment | ||
| * (i.e., not in the config) is passed through and a {@link RaiseExceptionErrorHandler} is provided. | ||
| * DUPE? I think this is testing RaiseExceptionErrorHandler. |
There was a problem hiding this comment.
That is correct. It is testing that exception handler works.
There was a problem hiding this comment.
Since that's the case I'm going to move this to a unit test.
There was a problem hiding this comment.
RaiseExceptionErrorHandlerTest unit test, which doesn't exist atm.
| @@ -1077,59 +634,29 @@ public void activateWithTypedAttributes() throws Exception { | |||
| * In this case, the activate call should not remove the unknown attribute from the given map. | |||
| */ | |||
| @Test | |||
| @Ignore("Kind of a silly test since the event factory strips these unknownAttributes out before sending.") | |||
There was a problem hiding this comment.
Why are we skipping this at the moment?
There was a problem hiding this comment.
Also, I think the intention of the test is that unknown attributes are stripped.
There was a problem hiding this comment.
The test was originally asserting that the unknownattribute were getting passed to the EventFactory:
// setup the attribute map captor (so we can verify its content)
ArgumentCaptor<Map> attributeCaptor = ArgumentCaptor.forClass(Map.class);
// verify that the event builder was called with the expected attributes
verify(mockEventFactory).createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment),
eq(bucketedVariation), eq(testUserId), attributeCaptor.capture());
Map<String, String> actualValue = attributeCaptor.getValue();
assertThat(actualValue, hasKey("unknownAttribute"));
Now the test is going to assert that it's stripped from the payload, so the semantics have changed a bit.
|
|
||
| Map<String, String> testUserAttributes = new HashMap<String, String>(); | ||
| if (datafileVersion >= 4) { |
There was a problem hiding this comment.
Alright, this if condition makes no sense.
There was a problem hiding this comment.
These Harry Potter references 🤦♂
There was a problem hiding this comment.
Yeah it might not be necessary at all. More of the copy-paste that needs to be purged.
| } | ||
|
|
||
| /** | ||
| * Verify that {@link Optimizely#track(String, String, Map)} | ||
| * dispatches events even if the event links only to launched experiments | ||
| */ | ||
| @Test | ||
| // @Ignore("Is this a bug??") |
There was a problem hiding this comment.
Not a bug. Launched state works like experiment is running, but impression event is not dispatched during activate.
There was a problem hiding this comment.
Need to remove that. I was investigating the test setup and got it working.
| } | ||
|
|
||
| /** | ||
| * Verify that {@link Optimizely#track(String, String, Map)} | ||
| * dispatches log events when the tracked event links to both launched and running experiments. | ||
| * | ||
| * TODO Is this test needed? Feels unnecessary in a post EET world. |
| assertEquals(testUserAttributes.get(entry.getKey()), entry.getValue()); | ||
| } | ||
|
|
||
| assertEquals(event.getRequestMethod(), RequestMethod.GET); |
There was a problem hiding this comment.
The mocked logEventToDispatch was a GET, removing the mock and using the actual EventFactory meant this needed to change to POST.
| @@ -1,6 +1,6 @@ | |||
| /** | |||
| * | |||
| * Copyright 2016-2017, Optimizely and contributors | |||
| * Copyright 2016-2017, 2019 Optimizely and contributors | |||
There was a problem hiding this comment.
Updated even when no change?
5169873 to
3472bfd
Compare
01e6ee5 to
0b2ada2
Compare
Summary
The overuse of Mockito has made the tests fairly brittle and also required a large amount of setup for each specific test. Moving to a more black box approach and verify the side-effects of the scenarios makes for much cleaner and more concise tests.
This is also a prerequisite for a larger change to the event processing in which we'll be batching requests to the log endpoint. If this refactor was not done, then we'd have to change all of the mock setups related to dispatching at that time which would erode confidence that we're maintaining parity when introducing that change.