-
Notifications
You must be signed in to change notification settings - Fork 733
[tests] Add AspireEventHub to playground tests #5319
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
Conversation
|
Depends on #5318 . |
|
Failing with this: The file is bind-mount'ed, and writing to it, or trying to create that directory fails on linux. |
|
Blocked on #5326 . |
|
Blocked on #5371 . |
|
/azp run |
|
This should be unblocked |
|
Azure Pipelines successfully started running 1 pipeline(s). |
9669ea4 to
5a8a987
Compare
5a8a987 to
f5a151d
Compare
…sCleanlyForAppsWithNoOtherTests
…the fallback test AppHostRunsCleanlyForAppsWithNoOtherTests
| var consumerMessageLoggedTask = | ||
| app.WaitForTextAsync(log => log.Contains(consumerMessage), resourceName: "consumer") | ||
| .WaitAsync(TimeSpan.FromMinutes(2)) | ||
| .ConfigureAwait(false); |
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.
Why is this declared up here? Why not just inline it in the await inside the try catch below?
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.
I'm trying to avoid missing this message.
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.
Doesn't it read through the whole log from the start? If it already happened, I think it would still get seen.
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.
You are correct, it does play the whole log from the start! Maybe it should be renamed to like FindOrWaitForTextAsync.
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.
It's "Waiting for this text to happen", if it has already happened, you are done waiting.
eerhardt
left a comment
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.
My last comment is pretty minor. Mostly a question for my understanding.
This change LGTM.
|
@radical I think the test is passing but there are some exceptions in the logs that look by design (retries?) |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
AppHostRunsCleanlyForAppsWithNoOtherTestsMicrosoft Reviewers: Open in CodeFlow