search

segment-integrations / analytics-android-integration-amplitude

The Amplitude analytics-android integration.
https://segment.com/docs/connections/destinations/catalog/amplitude/
2 stars 6 forks source link

Update Tests #43

Closed f2prateek closed 6 years ago

f2prateek commented 6 years ago

  1. Instead of using properties.toJSONObject(), use new JSONObject() where possible. This is makes it easy to read in the tests what the final JSONObject will look like. I only did this for empty properties for now, but would be good to do this for other ones as well.

  2. Split some tests. Anytime you call reset on a mock, it's generally a good idea to split that into multiple test cases.

  3. Use a hamcrest matcher Matchers.samePropertyValuesAs(expectedIdentify) instead of ArgumentCaptor to make tests a bit easier to read.

  4. Fix some lint warnings (e.g. deprecation).

SegmentBot commented 6 years ago

By analyzing the blame information on this pull request, we identified and undefined to be a potential reviewer.

brennan commented 6 years ago

Awesome - thanks. Looks like all V2 tests need integration.useLogRevenueV2 = true; because we still default this setting to false. That's why a few tests are failing. I can submit an update.

f2prateek commented 6 years ago

@brennan yep, just saw that and fixed!