Async functional tests #35

archolewa · 2016-09-08T15:40:16Z

Adds the functional tests for asynchronous queries.

In order to support this, I had to slightly modify the TestDruidServlet to have a Producer<String> that generates a fake Druid response on demand, rather than having the Druid response directly. This was needed to support delaying the Druid response, so that we could test the path where results aren't yet ready from the backend.

The base spec for the asynchronous queries is rather complex, and I still don't think I explained it well. Suggestions on improving the documentation for that test are especially welcome.

venisa · 2016-09-08T16:46:38Z

...re/src/main/java/com/yahoo/bard/webservice/async/jobs/payloads/DefaultJobPayloadBuilder.java

+        job.put(JOB_TICKET.getName(), jobRow.get(JOB_TICKET));
+        job.put(DATE_CREATED.getName(), jobRow.get(DATE_CREATED));
+        job.put(DATE_UPDATED.getName(), jobRow.get(DATE_UPDATED));
+        job.put(USER_ID.getName(), jobRow.get(USER_ID));


👍 I am having a 'Why didn't I do this moment'

There's almost always a better way to do something. That's why I love this review process, I get smarter by watching other people's work.

venisa · 2016-09-08T19:22:16Z

I just had a bunch of silly comments about new lines and whitespace. We should probably update the Javadoc for AsyncResultsReadySpec to say result 'SUCCESS' instead of result 'READY'. But apart from that 👍

dayamr · 2016-09-13T21:36:41Z

.../src/test/groovy/com/yahoo/bard/webservice/web/endpoints/BaseDataServletComponentSpec.groovy

@@ -139,15 +143,28 @@ abstract class BaseDataServletComponentSpec extends Specification {
        GroovyTestUtils.compareJson(result, expectedResult)
    }

+


dayamr · 2016-09-13T21:37:46Z

Apart from new line and extra line comments, it looks good to me. 👍

cdeszaq · 2016-09-14T18:34:18Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncDruidSendsErrorSpec.groovy

+
+    @Override
+    Map<String, Closure<String>> getResultsToTargetFunctions() {
+        return [


return not needed

/NotABlocker

cdeszaq · 2016-09-14T18:37:22Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncDruidSendsErrorSpec.groovy

+
+    @Override
+    Map<String, Closure<Void>> getResultAssertions() {
+        return [


return not needed

/NotABlocker

cdeszaq · 2016-09-14T18:39:19Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncDruidSendsErrorSpec.groovy

+
+    @Override
+    Map<String, Closure<Map<String, List<String>>>> getQueryParameters() {
+        return [


return not needed

/NotABlocker

cdeszaq · 2016-09-14T18:48:20Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncDruidSendsErrorSpec.groovy

+                ]},
+                jobs: {[:]},
+                results: {[:]},
+                syncResults: {


The ordering of this map should (for clarity at least) match the ordering of the other closure maps.

cdeszaq · 2016-09-14T18:49:25Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+ * @param <T>  The type to use for keys into the maps containing test-specific resources
+ */
+@Timeout(30)
+abstract class AsyncFunctionalSpec extends Specification {


You've not actually made this class generic, though the JavaDoc seems to indicate as such.

Ah, yes. Originally it was generic. Then I decided that allowing the user to provide some arbitrary type for keys just made a (already very abstract) class more abstract than it needed to be.

cdeszaq · 2016-09-14T18:54:44Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+    JerseyTestBinder jtb
+
+    /**
+     * Returns a map of closures, each of which transforms a map of named Responses into request targets.


Giving the expected method signature of these Closures (since the Closure type interface doesn't indicate it) would be good, I think. Based on this JavaDoc, it looks like it might be something like Map<String, Response> -> requestTarget, but it isn't clear.

Note that this "give us the signature" sentiment applies to the rest of these Closure maps.

cdeszaq · 2016-09-14T18:57:40Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+     * The key for each query parameter generating closure must correspond to the key of the associated target
+     *
+     * @return The map of query parameters for each target.
+     *


Blank comment line not needed

cdeszaq · 2016-09-14T18:58:56Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+            int statusCode = getDruidStatusCode()
+            jtb.nonUiDruidWebService.statusCode = statusCode
+            if (statusCode == 200) {
+                jtb.nonUiDruidWebService.jsonResponse = druidResponse


Should this be druidResponse.call()?

No. The jsonResponse in the DruidWebService is now a Function, not a String in order to support delaying the response.

Ok, makes sense. Need changelog entry for this.

cdeszaq · 2016-09-14T19:01:37Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+                    queryParameters.get(interactionName)(previousResponses)
+            )
+            response.bufferEntity()
+            resultValidations[interactionName](response)


Some comments in here around what's happening would be very helpful, especially for those less familiar with what calling functions retrieved via map access or method calls looks like (ie. myMap[foo](bar) or myMap.get(foo)(bar))

Also, we should use 1 map access style: either the index accessor as used on this line, or the getter as used 3 lines up. I like index access, personally, but we should use just one.

cdeszaq · 2016-09-14T19:02:51Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+            )
+            response.bufferEntity()
+            resultValidations[interactionName](response)
+            previousResponses.put(interactionName, response)


Could also use index accessor to "set" it, if you opt for that style of access

cdeszaq · 2016-09-14T19:10:33Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncDruidSendsErrorSpec.groovy

+    @Override
+    Map<String, Closure<Void>> getResultAssertions() {
+        return [
+                data: {


I feel like having comments in here indicating the scenario and what the assertion is expecting would help aid in understanding both what behavior is being tested for, as well as what the queries are for which the behavior is expected.

cdeszaq · 2016-09-14T19:13:59Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncInvalidApiRequest.groovy

+
+    @Override
+    Closure<String> getFakeDruidResponse() {
+        return {"I will never leave this curly prison."}


Rather than having to override a method when we don't care about the response, does it make sense to have the method be non-abstract with a default Druid response of an empty object, or list or something?

Personally, I think that depends on how often we expect to have tests that ignore the results from Druid. I don't like the idea of an exceptional case being the default. Every test that I've written except for this one do interact with Druid, so it's exceptional in this case, at least.

Now that I'm done using fancy speak that makes me sound like a craftsman, the real reason is that I'm lazy, and one nice thing about making it abstract is that with a few keystrokes in IntelliJ I can automatically generate method signatures for all the abstract methods. If I make this non-abstract, I can generate all the methods but one that I need to override, and then need to generate the getFakeDruidResponse method signature as a separate step.

Not a big deal, but I find it mildly annoying when overriding the BaseDataServletComponentSpec, and I figured that overriding the Druid responses would be common enough to justify making it abstract to save myself a few keystrokes.

Fair enough, I'm OK leaving it as abstract

cdeszaq · 2016-09-14T19:14:48Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncResultsNotReadySpec.groovy

+import javax.ws.rs.core.Response
+
+/**
+ * Verifies that if the result of an asynchronous query is not ready yet, then the user get back the exact same payload


cdeszaq · 2016-09-14T19:17:11Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncResultsNotReadySpec.groovy

+                jobs: {AsyncTestUtils.buildTicketLookup(it.data.readEntity(String))},
+                results: {payloads ->
+                    String dataPayload = payloads.data.readEntity(String)
+                    "jobs/${AsyncTestUtils.getJobFieldValue(dataPayload, DefaultJobField.JOB_TICKET.name)}/results"


A comment here around what's being looked at would be good.

cdeszaq · 2016-09-14T19:28:41Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncTestUtils.groovy

+   }
+
+   /**
+    * Given a String representation of the job information sent to the user, returns the URI path for a point lookup of


s/String/JSON

cdeszaq · 2016-09-14T19:46:29Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/ErrorDataServletSpec.groovy

@@ -138,7 +138,7 @@ class ErrorDataServletSpec extends Specification {
        setup:
        DruidServiceConfig oldConfig = nonUiTestWebService.serviceConfig
        nonUiTestWebService.serviceConfig = new DruidServiceConfig("Non-Ui Broker", oldConfig.url, 123, oldConfig.priority)
-        nonUiTestWebService.jsonResponse = standardGoodDruidResponse
+        nonUiTestWebService.jsonResponse ={standardGoodDruidResponse}


whitespace needed

cdeszaq · 2016-09-14T19:50:28Z

...re/src/test/java/com/yahoo/bard/webservice/models/druid/client/impl/TestDruidWebService.java

@@ -39,7 +40,7 @@
    );
    private static ObjectWriter writer = mapper.writer();

-    public String jsonResponse = "[]";
+    public Producer<String> jsonResponse = () -> "[]";


Definitely need Changelog entry for this, since it's a breaking change for anyone trying to use this.

cdeszaq · 2016-09-14T19:51:18Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncResultsReadySpec.groovy

+                            .collectEntries { parameter ->
+                                    def (String key, String value) = parameter.tokenize("=")
+                                    return [(key): value.tokenize(",")]
+                            }


Does it make sense to extract this bit as a helper method as well? I think I saw it at least 2 places in this PR and it's not the most easy-to-understand code under the sun.

cdeszaq · 2016-09-14T19:52:13Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncFunctionalSpec.groovy

+            int statusCode = getDruidStatusCode()
+            jtb.nonUiDruidWebService.statusCode = statusCode
+            if (statusCode == 200) {
+                jtb.nonUiDruidWebService.jsonResponse = druidResponse


Ok, makes sense. Need changelog entry for this.

archolewa · 2016-09-14T21:14:39Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncResultsNotReadySpec.groovy

+                SAME AS ABOVE
+    */
+
+    final CountDownLatch validationFinished = new CountDownLatch(3)


https://www.youtube.com/watch?v=9jK-NcRmVcw

cdeszaq · 2016-09-14T21:21:45Z

fili-core/src/test/groovy/com/yahoo/bard/webservice/async/AsyncResultsNotReadySpec.groovy

-            Thread.sleep(30000)
+            //We don't want the backend to return until all the validation has been performed, to simulate the backend
+            //taking a while to respond.
+            validationFinished.await()


cdeszaq

👍 Just needs to be rebased, squashed, and (assuming tests pass), merged

cdeszaq · 2016-09-14T21:25:15Z

Also, not sure why, but GitHub's PRs seem to be rather borked today. Everything I've commented on has either been addressed (though it doesn't show it), or was mitigated via comments.

archolewa · 2016-09-14T21:27:05Z

Yeah, I've noticed that too. It looks like there have been some changes to the UI since last I reviewed, so it's likely they've got some bugs because of their changes.

--Rather than storing the fake druid response as a JSON string, instead, the TestDruidWebService stores a `Producer` that generates the JSON string on demand. This allows tests to introduce delays (or perform other tasks) before returning the druid response. Quite useful when you want to test what happens when the results are not ready in an asynchronous query.

--Also added a few missing fields (`userId` and `dateUpdated`) to the job payload.

archolewa added NEED 2 REVIEWS REVIEWABLE labels Sep 8, 2016

venisa reviewed Sep 8, 2016
View reviewed changes

venisa added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Sep 8, 2016

dayamr reviewed Sep 13, 2016
View reviewed changes

dayamr added NEED CHANGES NEED REBASE and removed NEED 1 REVIEW REVIEWABLE labels Sep 13, 2016

archolewa force-pushed the async-functional-tests branch from a2142cb to e03ce89 Compare September 14, 2016 18:34

archolewa added MERGEABLE and removed NEED CHANGES NEED REBASE labels Sep 14, 2016

cdeszaq reviewed Sep 14, 2016

View reviewed changes

archolewa force-pushed the async-functional-tests branch from e03ce89 to 8d46e20 Compare September 14, 2016 18:59

cdeszaq reviewed Sep 14, 2016

View reviewed changes

cdeszaq requested changes Sep 14, 2016

View reviewed changes

archolewa commented Sep 14, 2016

View reviewed changes

archolewa force-pushed the async-functional-tests branch from 41d8dc5 to 31cb74d Compare September 14, 2016 21:15

cdeszaq reviewed Sep 14, 2016

View reviewed changes

cdeszaq approved these changes Sep 14, 2016

View reviewed changes

archolewa force-pushed the async-functional-tests branch from 31cb74d to b413b63 Compare September 14, 2016 21:44

Added asynchronous functional tests.

f32a541

--Also added a few missing fields (`userId` and `dateUpdated`) to the job payload.

archolewa force-pushed the async-functional-tests branch from b413b63 to f32a541 Compare September 14, 2016 22:03

archolewa merged commit f32a541 into master Sep 14, 2016

archolewa deleted the async-functional-tests branch September 14, 2016 22:11

		@@ -139,15 +143,28 @@ abstract class BaseDataServletComponentSpec extends Specification {
		GroovyTestUtils.compareJson(result, expectedResult)
		}

Async functional tests #35

Async functional tests #35

Conversation

archolewa commented Sep 8, 2016

venisa Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

venisa commented Sep 8, 2016 • edited Loading

Choose a reason for hiding this comment

dayamr commented Sep 13, 2016

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

cdeszaq Sep 14, 2016 • 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

cdeszaq Sep 14, 2016 • 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

archolewa Sep 14, 2016 • 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

cdeszaq left a comment

Choose a reason for hiding this comment

cdeszaq commented Sep 14, 2016

archolewa commented Sep 14, 2016

venisa Sep 8, 2016 •

edited

Loading

venisa commented Sep 8, 2016 •

edited

Loading

cdeszaq Sep 14, 2016 •

edited

Loading

cdeszaq Sep 14, 2016 •

edited

Loading

archolewa Sep 14, 2016 •

edited

Loading