-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Flaky DLP tests #2447
Fix Flaky DLP tests #2447
Conversation
f0a1d6c
to
a1846b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL - I suspect you'll need a tiny delay before canceling the job, but I could very much be wrong.
LGTM - if I am wrong.
try (DlpServiceClient client = DlpServiceClient.create()) { | ||
client.cancelDlpJob(request); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine if it works, but I'd bet these things can't happen right after the other. (some hysteresis).
(and it would be a flaky test again)
CancelDlpJobRequest request = CancelDlpJobRequest.newBuilder().setName(jobId).build(); | ||
try (DlpServiceClient client = DlpServiceClient.create()) { | ||
client.cancelDlpJob(request); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine if it works, but I'd bet these things can't happen right after the other. (some hysteresis).
(and it would be a flaky test again)
String jobId = output.split("Job created: ")[1].split("\n")[0]; | ||
CancelDlpJobRequest request = CancelDlpJobRequest.newBuilder().setName(jobId).build(); | ||
try (DlpServiceClient client = DlpServiceClient.create()) { | ||
client.cancelDlpJob(request); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine if it works, but I'd bet these things can't happen right after the other. (some hysteresis).
(and it would be a flaky test again)
@lesv I think you're right about the delay. It might create a race condition, so that it fails at the assertion before the job is created. I added a 3s delay before the assertion and cancelling, but I'm not sure if that's enough. What do you think? I feel like timing always introduces a bit of flakiness to tests. |
You aren't testing canceling, so delay / cancel @ 1s, 4s, & 10s- if it still fails, ignore it. Comment that we don't really care about canceling, and we are only doing it to conserve quota. |
* modified tests to cancel job * added delay to tests * added comment
* modified tests to cancel job * added delay to tests * added comment
Fixes: #2302, #2303, #2317
The tests keep timing out, so I followed the pattern implemented in the corresponding python samples to create a job and immediately cancel it.