-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add .NET functional tests to Mint #64
Conversation
A basic question how are we thinking of making dotnet tests run with mint? |
blocked till further discussion |
Not blocked anymore. As discussed, looks like there is a way to run .Net tests on mono and hence the current Docker image itself. @poornas will send the updated PR |
@poornas can you pls add Just to clarify there is work to be done here. |
this PR can be reviewed - 2 wrinkles though.
|
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.
Overall changes look good. Disposing MemoryStream needs to be handled in some places.
Test result:
No executable found matching command "dotnet-/mint/run/core/minio-dotnet/out/Minio.Functional.Tests.dll"
Console.WriteLine("[Bucket] Exception: {0}", e); | ||
Assert.Fail(); | ||
} | ||
await TearDown(minio, bucketName); |
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.
Shouldn't filestream
be disposed in the finally
block or alternately use using
block to implicitly dispose?
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.
@krisis, done.
private static int MB = 1024 * 1024; | ||
|
||
// Create a file of given size from random byte array | ||
private static String CreateFile(int size) |
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.
Remove local CreateFile
-
Preferably use streaming as done here https://github.com/minio/minio-py/pull/547/files#diff-123f6df576c99ae259903f693e5bc6a4R34
-
For tests where Static files are needed, use
/mint/data
files
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.
Another example would be this https://github.com/minio/minio-java/pull/586/files
@nitisht, updated PR to use streaming wherever possible. |
@harshavardhana ,@nitisht - PR's #146,#149 and #150 on minio-dotnet repo need to be merged to master and released before I can remove the local copy of Functional Tests in the mint/run/core/minio-dotnet directory. There are also a couple of TODO's to uncomment before this PR is merged to Mint.Until then, this PR is blocked but can be reviewed. |
@poornas as minio/minio-dotnet#146, minio/minio-dotnet#149, minio/minio-dotnet#150 are merged. Marking this as unblocked. If there is nothing to add here, we can review this. |
@nitisht , need this PR minio/minio-dotnet#162 to merge as well into master before we can run dotnet tests on mint. |
@poornas minio/minio-dotnet#162 is merged. Can we move ahead on this |
@poornas I am just running the Docker image generated by travis
Can you please try this and confirm if you're not getting the error. |
@nitisht , yes I see the error with the command you posted - for play the tests.sh script sets the MINT_DATA_DIR variable triggering the symlink bug. I updated my other PR for additional functional tests on dotnet sdk to also eliminate use of symlinks. |
@poornas can you pls fix the conflicts? We can take this in as well before we release Mint. |
8012bbd
to
2f5a932
Compare
@nitisht , conflicts were fixed.PTAL. |
build/minio-dotnet/install.sh
Outdated
set -e | ||
|
||
MINIO_DOTNET_SDK_PATH="$MINT_RUN_CORE_DIR/minio-dotnet" | ||
MINIO_DOTNET_SDK_VERSION="1.0.3" |
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 can probably follow the approach to automatically pull the latest SDK version, as done in other SDKs. Like this
MINIO_DOTNET_VERSION=$(curl -s https://api.github.com/repos/minio/minio-dotnet/releases/latest | jq -r .tag_name)
if [ -z "$MINIO_DOTNET_VERSION" ]; then
echo "unable to get minio-dotnet version from github"
exit 1
fi
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.
can we do this @poornas
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.
yes,done.
Tested locally run perfectly fine
However there is a stray line printed in
We need to remove this for the log to be parseable |
Sent minio/minio-dotnet#192 |
build/minio-dotnet/install.sh
Outdated
MINIO_DOTNET_SDK_PATH="$MINT_RUN_CORE_DIR/minio-dotnet" | ||
MINIO_DOTNET_SDK_VERSION="1.0.3" | ||
|
||
outDir="$MINIO_DOTNET_SDK_PATH/out" |
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.
out_dir is more idiomatic (Ref: https://google.github.io/styleguide/shell.xml#Variable_Names)
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.
ok, changed this.
apt-transport-https | ||
dotnet-dev-1.0.4 | ||
nuget | ||
libunwind8 |
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.
new line missing
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.
fixed.
@@ -4,3 +4,5 @@ python3-pip | |||
ruby-dev | |||
ruby-bundler | |||
default-jdk | |||
dotnet | |||
nuget |
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.
new line missing
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.
fixed
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.
LGTM & Tested
@poornas can you fix this issue?
|
57b116d
to
ca2c01b
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.
Travis build failures need to be fixed..
Built and tested locally, works fine
|
Functional tests for minio-dotnet Sdk.