-
Notifications
You must be signed in to change notification settings - Fork 462
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
Extract Aspire.Hosting.MongoDB.Tests project #5017
Conversation
c42fba0
to
a6b8a9a
Compare
Maybe tests have feelings and don't like me. |
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.
What's the status of this PR? Is anyone working on getting the tests passing in CI?
//mongodb shutdown has delay,so without delay to running instance using same data and second instance failed to start. | ||
await Task.Delay(TimeSpan.FromSeconds(10)); |
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.
We need to make this more deterministic. Waiting for X seconds is usually how we get random/flakey failures.
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.
What's your suggestion?
I don't have any idea.
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.
Try to connect to the database until it fails?
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.
Is there any helper method exists to check container is fully stopped?
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 was expecting something like #4878. If you want to prototype trying to make that work. cc @karolz-ms @DamianEdwards
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.
@sebastienros - should we log an issue for this wait case, and then we can merge this PR?
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.
Filed #5199
Co-authored-by: Eric Erhardt <[email protected]>
I will check. |
Seems the test failed on Linux how can i see container logs? |
@eerhardt I can't open it! |
Hmm, does it work if you "log in" (using any Microsoft account)? |
Here's the log output:
|
didn't work |
Thanks, maybe i need logs again. |
I got an answer from some of my team members that if you aren't a Microsoft employee, you can download the whole artifacts by click the little three dots on the right: That will get you all the artifacts for that leg. |
…to mongodb-tests
tests/Aspire.Hosting.MongoDB.Tests/Aspire.Hosting.MongoDB.Tests.csproj
Outdated
Show resolved
Hide resolved
|
||
if (!OperatingSystem.IsWindows()) | ||
{ | ||
System.Diagnostics.Process.Start("/usr/bin/env", $"sudo chmod -R a+rwx {bindMountPath}").WaitForExit(); |
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 can't this use File.SetUnixFileMode?
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.
Maybe we shouldn't use Directory.CreateTempSubdirectory()
in this case since we need other users to be able to see it? Directory.CreateTempSubdirectory()
explicitly only gives the current user access to the directory, for security reasons. We might as well make a normal folder if we need other users to access it.
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 only concern is the "wait 10 seconds" in the test. But I think we could log an issue for it to unblock merging this PR.
Contributes to #3185
Contributes to #4294
Fixes #5184
Microsoft Reviewers: Open in CodeFlow