test(dialogflow): deflake and re-enable test cases in ITSystemTest#13511
test(dialogflow): deflake and re-enable test cases in ITSystemTest#13511whowes wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enables previously ignored integration tests (detectIntentTest and listContextsTest) in ITSystemTest and introduces the awaitility library to handle eventual consistency. The feedback suggests improving the robustness of these tests against flakiness. Specifically, for detectIntentTest, it is recommended to use untilAsserted to poll and retry all assertions together, which also removes the need for AtomicReference boilerplate. For listContextsTest, wrapping the assertion in an awaitility block is advised to prevent failures caused by replication lag.
| AtomicReference<DetectIntentResponse> responseRef = | ||
| new AtomicReference<>(); | ||
| await() | ||
| .atMost(Duration.ofSeconds(10)) | ||
| .pollInterval(Duration.ofSeconds(2)) | ||
| .until( | ||
| () -> { | ||
| DetectIntentResponse response = sessionsClient.detectIntent(request); | ||
| responseRef.set(response); | ||
| return !response.getQueryResult().getQueryText().isEmpty(); | ||
| }); | ||
| QueryResult result = responseRef.get().getQueryResult(); | ||
| assertEquals(EVENT_NAME, result.getQueryText()); | ||
| assertEquals(ACTION_NAME, result.getAction()); | ||
| assertEquals(DEFAULT_LANGUAGE_CODE, result.getLanguageCode()); | ||
| assertEquals(intent.getDisplayName(), result.getIntent().getDisplayName()); |
There was a problem hiding this comment.
Using until(...) with a condition that only checks if queryText is not empty might not actually prevent flakiness. Since the query input is an event, the query text in the response is immediately populated with the event name on the first attempt, even if the intent itself hasn't been fully propagated or matched yet. This causes the await() block to exit immediately, and the subsequent assertions (like matching the intent display name) might still fail due to eventual consistency.
Instead, use untilAsserted(...) to poll and retry all assertions. This also eliminates the need for the AtomicReference boilerplate.
| AtomicReference<DetectIntentResponse> responseRef = | |
| new AtomicReference<>(); | |
| await() | |
| .atMost(Duration.ofSeconds(10)) | |
| .pollInterval(Duration.ofSeconds(2)) | |
| .until( | |
| () -> { | |
| DetectIntentResponse response = sessionsClient.detectIntent(request); | |
| responseRef.set(response); | |
| return !response.getQueryResult().getQueryText().isEmpty(); | |
| }); | |
| QueryResult result = responseRef.get().getQueryResult(); | |
| assertEquals(EVENT_NAME, result.getQueryText()); | |
| assertEquals(ACTION_NAME, result.getAction()); | |
| assertEquals(DEFAULT_LANGUAGE_CODE, result.getLanguageCode()); | |
| assertEquals(intent.getDisplayName(), result.getIntent().getDisplayName()); | |
| await() | |
| .atMost(Duration.ofSeconds(10)) | |
| .pollInterval(Duration.ofSeconds(2)) | |
| .untilAsserted( | |
| () -> { | |
| DetectIntentResponse response = sessionsClient.detectIntent(request); | |
| QueryResult result = response.getQueryResult(); | |
| assertEquals(EVENT_NAME, result.getQueryText()); | |
| assertEquals(ACTION_NAME, result.getAction()); | |
| assertEquals(DEFAULT_LANGUAGE_CODE, result.getLanguageCode()); | |
| assertEquals(intent.getDisplayName(), result.getIntent().getDisplayName()); | |
| }); |
| boolean contextNameInActualContext = | ||
| stream(contextsClient.listContexts(request).iterateAll()) | ||
| .anyMatch(actualContext -> context.getName().equals(actualContext.getName())); | ||
| assertTrue(contextNameInActualContext); |
There was a problem hiding this comment.
To ensure listContextsTest is fully deflaked against eventual consistency (replication lag), we should also wrap the assertion in an awaitility block. Otherwise, if the context is not immediately visible in the list, the test will fail.
| boolean contextNameInActualContext = | |
| stream(contextsClient.listContexts(request).iterateAll()) | |
| .anyMatch(actualContext -> context.getName().equals(actualContext.getName())); | |
| assertTrue(contextNameInActualContext); | |
| await() | |
| .atMost(Duration.ofSeconds(10)) | |
| .pollInterval(Duration.ofSeconds(2)) | |
| .untilAsserted( | |
| () -> { | |
| boolean contextNameInActualContext = | |
| stream(contextsClient.listContexts(request).iterateAll()) | |
| .anyMatch(actualContext -> context.getName().equals(actualContext.getName())); | |
| assertTrue(contextNameInActualContext); | |
| }); |
a95787c to
0b301eb
Compare
Each of these test cases failed 7/30 times locally without these fixes. With the fixes applied, tests pass without flakes in 100+ runs.
detectIntentTest() flaked regularly if the intent created in setUp() didn't have sufficient time to fully propagate before detectIntent() was called in the test case. Poll until detectIntent() returns a non-empty result before proceeding to assertion on the content.
listContextsTest() flaked if a system context was encountered (see https://docs.cloud.google.com/dialogflow/es/docs/reference/rest/v2beta1/projects.agent.environments.users.sessions.contexts/patch#path-parameters). Adjust the flaky assertion to only look for the expected context while tolerating others that happen to be present.