Fix DialogFlow tests and update to canonical sample format. by dzlier-gcp · Pull Request #1210 · GoogleCloudPlatform/java-docs-samples

dzlier-gcp · 2018-09-11T17:05:14Z

DialogFlow tests are failing, and while fixing them I ran into many issues with the tests simply parsing the stdout content to verify things were working as intended.

With canonical samples, we are no longer formatting the samples as commandline executables, and instead of writing to stdout, we are returning the requested information directly, as that is more likely to be what users are looking for anyway. So I updated the DF samples to do so, and the tests to match.

beccasaurus

We won't be accepting changes to ML API docs samples which remove the output response information, it's critical to the user's docs experience.

We can't accept this change.

dzlier-gcp · 2018-09-11T18:04:22Z

More context on @Beccca 's response:

The issue is with deleting the System.out.println lines in the classes, as these are determined by the DPEs in collaboration with the product teams to demonstrate the format of the responses returned from the various API calls in the code snippets embedded in our documentation. When thinking about the canonical format, I interpreted the decision that "we no longer have to make CLI's/executables" to mean there was no need to write to stdout if the code is not an executable, but that wasn't the main reason for the write lines in the first place. I will be adding those back in.

The issue I wanted to resolve in this change was making the functions actually return something other than void, for 2 reasons:

Unit tests that capture and regex match on stdout are brittle, prone to problems, and don't do much to verify that the call was actually successful, particularly when it's only matching on the manual string part and not on the content returned itself.
Functions like "list_" and "get_" that return void are functionally useless for anything other than documentation - they can't be copied into a developer's project and used as-is, because they don't do anything actually useful in practice.

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java

dzlier-gcp · 2018-09-11T18:33:23Z

Updated to return the System.out lines in the sample classes.

Requested changes made.

kurtisvg · 2018-09-11T20:59:36Z

dialogflow/cloud-client/src/main/java/com/example/dialogflow/ContextManagement.java

   */
-  public static void listContexts(String sessionId, String projectId) throws Exception {
+  public static List<Context> listContexts(String sessionId, String projectId) throws Exception {
+    List<Context> contexts = Lists.newArrayList();


I think if we are aiming for the new format, the context tags need to be moved inside of the function and examples of the arguments need to be provided in comments to give the user additional context:

public static List<Context> listContexts(String sessionId, String projectId) throws Exception { // [START dialogflow_list_contexts] // String sessionId = "my-session-id" // String projectId = "my-project-id" List<Context> contexts = Lists.newArrayList();

What's your take on the return statement being inside vs outside the tags?

I'd say leave the return outside.

I think that sounds reasonable.

From the perspective of having these regions be copy-pastable by developers, though, I would think the function tag and return statement would be useful so that they could just plop the function into their code directly. They would basically have to write a function wrapper around the code anyway regardless

I see what you are saying, but I think the presumption is that they can copy paste the code into an existing code and get it working. Even if they copy the function as a whole they still have to create a class around it, so I don't think including the function signature really make's a significant impact.

hey every keystroke counts :)

I feel we should make sure this gets decided in the new rubric. (To have a solid answer going forward)

Agreed. I'd prefer to leave it as-is for now though for consistency.

kurtisvg · 2018-09-11T21:05:19Z

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java

          System.out.format(" - Confidence: '%s'\n", answer.getMatchConfidence());
        }
+
+        allKnowledgeAnswers.add(Tuple.of(text, sessionsClient


Another suggestion on the new sample guidelines is to explicitly define variables ahead of time, and avoid multiple nesting in function calls.

Updated to break out nested calls.

beccasaurus

I had submitted a Request changes – after chatting & agreeing that the println statements would return, I'm 'Approving' to remove the request!

Thanks for all of the thought that went into these changes to improve our code sample experience for developers 🙏

=> @nnegrey for full approval from our end

nnegrey

Mostly nit to match the naming to the API naming

dialogflow/cloud-client/src/test/java/com/example/dialogflow/KnowledgeBaseManagementIT.java

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DocumentManagement.java

dialogflow/cloud-client/src/main/java/com/example/dialogflow/KnowledgeBaseManagement.java

dialogflow/cloud-client/src/test/java/com/example/dialogflow/KnowledgeBaseManagementIT.java

Changes made.

dzlier-gcp · 2018-09-17T17:47:22Z

Hi all, I'm having an issue with this that I'm not sure I should report to the DialogFlow team directly as an actual bug found by the Unit Tests.

For the tests in KnowledgeBaseManagementIT, particularly the testIntentKnowledge, it creates a KnowledgeBase based on the Storage Docs FAQ.

The test question used to be "Where is my data stored?", which is the second question in the Storage section. Half the time, it was interpreting that as "Is my data redundant?", the first question in that section. When I changed the test text to "Is my data redundant?", it always succeeded locally, but now it's having the same problem run with kokoro where it thinks the question is the other one half the time.

This seems like a potential API problem though, since the input text is an exact match for one of the questions. Should I file a bug with the team, or just make the test more forgiving?

nnegrey · 2018-09-17T17:53:59Z

I'd start by reaching out to DF team and see if something changed on the backend, this sounds like an API bug.

dzlier-gcp · 2018-09-17T18:01:21Z

Created bug b/115888589, but there was no automatic assignee so I'll forward it to the dialogflow team to make sure they see it.

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java

madongfly · 2018-09-17T23:07:41Z

dialogflow/cloud-client/src/test/java/com/example/dialogflow/KnowledgeBaseManagementIT.java

+      assertEquals(document.getName(), answer.getSource());
+      assertThat(answer.getAnswer()).contains("Cloud Storage");
+    }
+  }


Please cleanup the resources (remove the knowledge base) after the test.

This is done in the @After tearDown() method above, line 77.

nnegrey

LGTM

Thanks Dane!

beccasaurus · 2018-09-27T20:28:13Z

ping ~ what's the status of this? looks like we've got LGTMs :)

dzlier-gcp · 2018-09-27T22:58:05Z

Ah, thanks for the ping, I missed the LGTM. I will update the branch and submit.

…1210)

dzlier-gcp requested review from kurtisvg and nnegrey September 11, 2018 17:05

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 11, 2018

beccasaurus previously requested changes Sep 11, 2018

View reviewed changes

nnegrey reviewed Sep 11, 2018

View reviewed changes

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java Outdated Show resolved Hide resolved

dzlier-gcp force-pushed the master branch 2 times, most recently from ffc98db to 259c6c3 Compare September 11, 2018 18:32

dzlier-gcp force-pushed the master branch from 259c6c3 to 60e5245 Compare September 11, 2018 19:07

dzlier-gcp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 11, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 11, 2018

kurtisvg reviewed Sep 11, 2018

View reviewed changes

beccasaurus previously approved these changes Sep 11, 2018

View reviewed changes

dzlier-gcp dismissed beccasaurus’s stale review via 3f38b03 September 11, 2018 21:36

dzlier-gcp force-pushed the master branch 3 times, most recently from a440107 to a588ef6 Compare September 12, 2018 18:55

dzlier-gcp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2018

nnegrey previously requested changes Sep 13, 2018

View reviewed changes

dzlier-gcp force-pushed the master branch from 643315a to 92ff036 Compare September 13, 2018 17:54

dzlier-gcp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 13, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 13, 2018

dzlier-gcp force-pushed the master branch from e16c22b to d3b3d5a Compare September 13, 2018 21:13

dzlier-gcp added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2018

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 17, 2018

madongfly reviewed Sep 17, 2018

View reviewed changes

dialogflow/cloud-client/src/main/java/com/example/dialogflow/DetectIntentKnowledge.java Outdated Show resolved Hide resolved

madongfly reviewed Sep 17, 2018

View reviewed changes

Fix DialogFlow tests and update to canonical sample format.

3a6f97d

dzlier-gcp force-pushed the master branch from b8f2033 to 3a6f97d Compare September 18, 2018 16:18

Merge branch 'master' into master

6ca5ea1

nnegrey approved these changes Sep 20, 2018

View reviewed changes

Merge branch 'master' into master

5371e19

Merge branch 'master' into master

585daa0

dzlier-gcp merged commit 2c1a8d7 into GoogleCloudPlatform:master Sep 27, 2018

Shabirmean pushed a commit that referenced this pull request Nov 14, 2022

samples: Fix DialogFlow tests and update to canonical sample format. (#…

9c50376

…1210)

Shabirmean mentioned this pull request Nov 15, 2022

samples: migrate code from googleapis/java-dialogflow #7420

Merged

9 tasks

Shabirmean pushed a commit that referenced this pull request Nov 15, 2022

samples: Fix DialogFlow tests and update to canonical sample format. (#…

c644c5c

…1210)

Conversation

dzlier-gcp commented Sep 11, 2018

Uh oh!

beccasaurus left a comment

Choose a reason for hiding this comment

Uh oh!

dzlier-gcp commented Sep 11, 2018

Uh oh!

Uh oh!

dzlier-gcp commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beccasaurus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnegrey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dzlier-gcp commented Sep 17, 2018

Uh oh!

nnegrey commented Sep 17, 2018

Uh oh!

dzlier-gcp commented Sep 17, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nnegrey left a comment

Choose a reason for hiding this comment

Uh oh!

beccasaurus commented Sep 27, 2018

Uh oh!

dzlier-gcp commented Sep 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

beccasaurus left a comment •

edited

Loading