Fix DialogFlow tests and update to canonical sample format.

[dzlier-gcp]

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]

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:

1. 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.
2. 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.

[dzlier-gcp]

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

Requested changes made.

[kurtisvg]

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();

[dzlier-gcp]

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

[nnegrey]

I'd say leave the return outside.

[kurtisvg]

I think that sounds reasonable.

[dzlier-gcp]

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

[kurtisvg]

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.

[dzlier-gcp]

hey every keystroke counts :)

[nnegrey]

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

[dzlier-gcp]

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

[kurtisvg]

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

[dzlier-gcp]

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

Changes made.

[dzlier-gcp]

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]

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]

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.

[madongfly]

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

[dzlier-gcp]

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

[nnegrey]

LGTM

Thanks Dane!

[beccasaurus]

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

[dzlier-gcp]

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