Add a bunch of integration tests for Metric, Funnel, and MultiAnalysis #127

baumatron · 2017-10-20T03:11:20Z

This tests a bunch of code that previously wasn't tested in automation. For this round I focused on getting all types of queries tested with some basic tests. More tests still need to be added to cover other parts of the SDK, but this was a major area that needed work. Takes code coverage from 57% to 70%.

It's interesting that some of the abstractions that came around like QueryParameters reflect some of the ideas that have been discussed around reshaping the SDK surface by getting rid of all of the different overloads of Metric, etc. I'd like to see something even more generic, and I'd bet we can reduce Queries.cs to possibly even a single generic method that takes a QueryParameters type object and uses Json.Net to deserialize results into concrete types where we use JsonConverters where those types need to be transformed in some way.

…uire it

…properties

Also make tests indifferent to order of query string parameters

…ll bug

coveralls · 2017-10-20T03:13:15Z

Coverage increased (+13.03%) to 70.355% when pulling 1e3fc36 on increase_coverage_2 into b820c2e on master.

coveralls · 2017-10-20T18:30:32Z

Coverage increased (+13.1%) to 70.446% when pulling d29f8a7 on increase_coverage_2 into b820c2e on master.

masojus

Looks awesome! I left comments, none of which are major enough to block merging this, so use your own discretion and file issues for stuff you deem out of scope for this change.

masojus · 2017-10-21T04:52:47Z

Keen.NetStandard/Query/Queries.cs

@@ -312,18 +312,28 @@ internal class Queries : IQueries
            return o;
        }

-        public async Task<IDictionary<string, string>> MultiAnalysis(string collection, IEnumerable<MultiAnalysisParam> analysisParams, IQueryTimeframe timeframe = null, IEnumerable<QueryFilter> filters = null, string timezone = "")
+        string SerializeMultiAnalysisQueryParameter(IEnumerable<MultiAnalysisParam> analysisParams)


Maybe ...Params or ...Parameters plural since it's a collection of them?

Parameter here is referring to the analyses parameter. In context it seems to make sense:

parms.Add(KeenConstants.QueryParmAnalyses, SerializeMultiAnalysisQueryParameter(analysisParams));

I'll have a look though and see if I can come up with a better name.

Also I hate the abbreviation parms. We should change that someday.

I see, yeah that makes sense. I don't like parms either--I'd always rather spell it out or go with params which seems like a more commonly accepted convention. If I ctr+shift+F I'll always try "param" and then I'll find either param or params or parameter/parameters but not parms.

masojus · 2017-10-21T04:53:44Z

Keen.NetStandard/Query/TimeframeConverter.cs

@@ -46,7 +46,7 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s
            // absolute timeframe.
            if (JTokenType.String == jsonToken.Type)
            {
-                return QueryRelativeTimeframe.Create(jsonToken.ToString(Formatting.None));
+                return QueryRelativeTimeframe.Create(jsonToken.Value<string>());


Did you happen to test this against the real back end?

Was a while ago that I made this change, but I think so.

Also, I highly doubt this is actually used anywhere in the SDK for deserialization. What do you think?

From when I fixed Issue #50 in commit 70fba95 I'd expect it to get used, at least maybe when deserializing FunnelResultStep but I'd have to go back and look more carefully. Does it not get hit in the Funnel_ValidTimeframeInSteps_Success() test?

masojus · 2017-10-21T05:06:13Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+            var actualQueries = await client.GetQueries();
+
+            Assert.AreEqual(expectedQueries.Count, actualQueries.Count());
+            foreach (var expectedQuery in expectedQueries)


Could maybe do something like AssertSequenceEquals() here or something based on LINQ's SequenceEqual(), the stuff at MoreLINQ will give better failure messages.

We should also be making use of either the classic model CollectionAssert helpers or the newer Collection Constraints stuff where applicable. In general we don't use much of the constraints-based stuff in these tests, but now that we're on a newer NUnit, we may as well use it.

Seems way better, I'll change that throughout.

masojus · 2017-10-21T05:27:21Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+        {
+            var parameters = queryParameters.GetQueryParameters();
+            StringBuilder queryStringBuilder = new StringBuilder();
+            foreach (var parameter in parameters)


Is this better or worse than what we do elsewhere:

var parmVals = (parms == null) ? "" : string.Join("&", from p in parms.Keys where !string.IsNullOrEmpty(parms[p]) select string.Format("{0}={1}", p, Uri.EscapeDataString(parms[p])));

I don't really like either that much, but I found another way to do what I want with something like this:

// Create an empty NameValueCollection var expectedQueryStringCollection = new NameValueCollection(); // Add all expected parameters to the collection foreach (var parameter in queryParameters.GetQueryParameters()) { if (null != parameter.Value) { expectedQueryStringCollection[parameter.Key] = parameter.Value; } }

which seems more readable and isn't creating the superfluous query string to then be parsed later.

masojus · 2017-10-21T05:33:54Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+
+                    Assert.AreEqual(expectedPath, actualPath);
+
+                    var expectedQueryStringCollection = HttpUtility.ParseQueryString(queryStringBuilder.ToString());


Is there good reason to build a StringBuilder and then parse it rather than directly build up a NameValueCollection?

Not anymore, I've changed this.

Great, thanks!

masojus · 2017-10-21T05:36:26Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+
+            internal virtual string GetResourceName() => Analysis;
+
+            internal virtual List<KeyValuePair<string, string>> GetQueryParameters()


Why a list of KVPs instead of a Dictionary<>? It seems like it just makes the notation more verbose.

Initially I was relying on the keys being in a particular order. Can probably change it to a Dictionary now.

I think there's an OrderedDictionary class or something similar if that's what we need, and then you'd still have O(1) lookup.

No need for having it ordered anymore as I've changed the tests to more properly ignore any difference in ordering of query string parameters.

masojus · 2017-10-21T05:55:08Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+            Assert.AreEqual(expectedResults.Count(), actualResults.Count());
+            var expectedEnumerator = expectedResults.GetEnumerator();
+            var actualEnumerator = actualResults.GetEnumerator();
+            while (expectedEnumerator.MoveNext() && actualEnumerator.MoveNext())


Isn't there some form of Zip() and/or All() to avoid grabbing the actual enumerators? Something like expectedResults.Zip(actualResults, (expected, actual) => Assert.AreEqual(expected.timeframe.Start.....

Also if we implemented equality for some of these types (like QueryRelativeTimeframe here) these sorts of checks would be easier, which I've been waiting for a reason to do anyway. Maybe file an issue?

masojus · 2017-10-21T06:00:46Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+
+            Assert.AreEqual(expectedResults.Count(), actualResults.Result.Count());
+            var actualEnumerator = actualResults.Result.GetEnumerator();
+            foreach (var expected in expectedResults)


This sort of iteration implies ordering...do we have reason to expect ordering will always be maintained? We saw that in a version to version change in Java where a particular collection was optimized and no longer maintained a certain ordering that some of the tests relied on, so it succeeded in Java 6 and 7 but broke when we tested on JDK 8 :/

This is actually an ordered collection, so this should be fine.

masojus · 2017-10-21T06:03:48Z

Keen.NetStandard.Test/QueryTests_Integration.cs

+
+            Assert.AreEqual(expectedResults.Count(), actualResults.Count());
+            var actualResultsEnumerator = actualResults.GetEnumerator();
+            foreach (var expectedResult in expectedResults)


Can JToken.DeepEquals() help in these cases so as to not manually delve into the entire JSON structure, or is it too rigid in that we want to ignore some extraneous properties or something?

Maybe, but we'll have to write just as much code probably to transform the actual result into something that will actually be comparable. For example, the SDK returns results as strings while the JSON returned is a number.

coveralls · 2017-11-06T18:00:50Z

Coverage increased (+12.7%) to 70.054% when pulling b68f3dd on increase_coverage_2 into b820c2e on master.

masojus · 2017-11-06T23:56:35Z

Looks good to me, and after re-running, the Travis builds succeeded, so merge away!

baumatron added 19 commits October 11, 2017 19:56

Add an integration test for getting available queries

d195f0a

backport new test

e99f285

Add tests for TimeframeConverter. Fix a bug in deserialization.

32ac186

Backport tests and fix.

28a75e1

A couple of simple analysis tests

1b1f20e

Simple test for grouped average

c39c96f

Backport tests

bb63150

Add a test for queries with an interval specified

b5df44b

Extract some common boilerplate

73a0378

Add a test for group by over an interval

adf73a4

Tests to cover producing select unique results

66e4021

Test for extraction

3b9bf32

Test for funnel

3528948

Multi-analysis test

0e7f749

Fix bug where target_property is included for analyses that don't req…

438709c

…uire it

Muti-analysis with group_by along with fix for including null target …

0554327

…properties

Test multi-analysis with an interval

3607e17

Add test for multi-analysis with interval and group by

2140d03

Also make tests indifferent to order of query string parameters

Backport additional tests, fix target_property being included when nu…

1e3fc36

…ll bug

De-jankify QueryRelativeTImeframe deserialization a bit

d29f8a7

masojus approved these changes Oct 21, 2017

View reviewed changes

Some PR Feedback

b68f3dd

baumatron merged commit 676e52f into master Nov 7, 2017

baumatron deleted the increase_coverage_2 branch November 7, 2017 00:50

This was referenced Nov 7, 2017

Datasets #128

Merged

Automated Unit Tests Have Poor Coverage #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a bunch of integration tests for Metric, Funnel, and MultiAnalysis #127

Add a bunch of integration tests for Metric, Funnel, and MultiAnalysis #127

baumatron commented Oct 20, 2017 •

edited

Loading

coveralls commented Oct 20, 2017

coveralls commented Oct 20, 2017

masojus left a comment

masojus Oct 21, 2017

baumatron Nov 3, 2017 •

edited

Loading

baumatron Nov 3, 2017

masojus Nov 3, 2017

masojus Oct 21, 2017

baumatron Nov 6, 2017

baumatron Nov 6, 2017

masojus Nov 6, 2017

masojus Oct 21, 2017

masojus Oct 21, 2017

baumatron Oct 30, 2017

masojus Oct 21, 2017

baumatron Nov 2, 2017 •

edited

Loading

masojus Oct 21, 2017

baumatron Nov 2, 2017

masojus Nov 3, 2017

masojus Oct 21, 2017

baumatron Oct 30, 2017

masojus Nov 1, 2017

baumatron Nov 2, 2017

masojus Oct 21, 2017

masojus Oct 21, 2017

masojus Oct 21, 2017

baumatron Nov 6, 2017

masojus Oct 21, 2017

baumatron Nov 6, 2017

coveralls commented Nov 6, 2017

masojus commented Nov 6, 2017


		Assert.AreEqual(expectedPath, actualPath);

		var expectedQueryStringCollection = HttpUtility.ParseQueryString(queryStringBuilder.ToString());


		internal virtual string GetResourceName() => Analysis;

		internal virtual List<KeyValuePair<string, string>> GetQueryParameters()

Add a bunch of integration tests for Metric, Funnel, and MultiAnalysis #127

Add a bunch of integration tests for Metric, Funnel, and MultiAnalysis #127

Conversation

baumatron commented Oct 20, 2017 • edited Loading

coveralls commented Oct 20, 2017

coveralls commented Oct 20, 2017

masojus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baumatron Nov 3, 2017 • 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

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

baumatron Nov 2, 2017 • 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

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

Choose a reason for hiding this comment

coveralls commented Nov 6, 2017

masojus commented Nov 6, 2017

baumatron commented Oct 20, 2017 •

edited

Loading

baumatron Nov 3, 2017 •

edited

Loading

baumatron Nov 2, 2017 •

edited

Loading