1 [](start = 97, length = 1)

1 [](start = 97, length = 1)

super-nit: Make a static variable. It helps self document the code. #Closed

creating a local variable

In reply to: 194554180 [](ancestors = 194554180)

const static?

In reply to: 194580367 [](ancestors = 194580367,194554180)

static wont apply to local variable. Not sure if you wanted to make this a class variable. I have used const only

In reply to: 195218006 [](ancestors = 195218006,194580367,194554180)

Exists(uri) && [](start = 19, length = 14)

Exists(uri) && [](start = 19, length = 14)

Call Exists doubles the number of network calls. Can we just rely on the GetDirectoryEntry alone? #Closed

removed

In reply to: 194554633 [](ancestors = 194554633)

This fixes the double network call but the catch all could hide simple exceptions like a network error.

In reply to: 194580374 [](ancestors = 194580374,194554633)

change reverted

In reply to: 195218945 [](ancestors = 195218945,194580374,194554633)

Why was the change reverted? #Resolved

_adlsClient.GetDirectoryEntry(uri.AbsolutePath) throws exception if uri not present

In reply to: 195886246 [](ancestors = 195886246)

So if you look at the source for adls client, https://github.com/Azure/azure-data-lake-store-net/blob/master/AdlsDotNetSDK/ADLSClient.cs

CheckExists internally call GetDirectoryEntry, so you are calling it twice. Can you model what the CheckExists code does internally? That may help with handling the specific case.

In reply to: 196918926 [](ancestors = 196918926,195886246)

fixed

In reply to: 197270432 [](ancestors = 197270432,196918926,195886246)

.StartsWith("1"); [](start = 49, length = 17)

Elaborate this check. What if it the result is "11"? #Closed

fixed

In reply to: 194555273 [](ancestors = 194555273)

SetupBlobs [](start = 37, length = 10)

nit: SetupBlobs doesn't really let me know what this function is doing. I suggest renaming something more explanitory and also exposing the blob count as well. maybe something like CreateTempBlobs(string directory, int blobCount=3) #Closed

fixed

In reply to: 194555964 [](ancestors = 194555964)

supernit: try to add newlines to break out the logic. Giant block of code are difficult to follow. #Closed

fixed

In reply to: 194556435 [](ancestors = 194556435)

This cleanup should be handled in the test cleanup.

Existing tests follow this same pattern

In reply to: 194556624 [](ancestors = 194556624)

Agreed, but please avoid in the future if possible. Please break up this code block for readability as well.

In reply to: 194580410 [](ancestors = 194580410,194556624)

fixed

In reply to: 195218460 [](ancestors = 195218460,194580410,194556624)

I know this is following the existing pattern -- but instead of having to remove all the skips and then set credentials, can we change this to dynamically skip the test if the credentials are not set? It looks like xunit does support a SkipTestException: https://github.com/xunit/samples.xunit/blob/master/DynamicSkipExample/SkipTestException.cs

Not sure if this is supported in our version of xunit or not. #Resolved

I would also put the check in a single function and call it before the test. #Resolved

SkippableFact is not supported in our version of xunit.
Yopu dont need to change each SkipMessage.Just set SkipMessage to null at top and all tests will run. These tests can only be run locally and a valid connectionstring is required to be inserted. I dont see how making this dynamic would be any help since the conn string validation will be run everytime in a build pipeline and it will always be expected to fail

In reply to: 195885915 [](ancestors = 195885915)

This value can only be a const string.
You dont need to change each SkipMessage.Just set SkipMessage to null at top and all tests will run. These tests can only be run locally and a valid connectionstring is required to be inserted. I dont see how making this dynamic would be any help since the conn string validation will be run everytime in a build pipeline and it will always be expected to fail

In reply to: 195886005 [](ancestors = 195886005)

OK sounds good, it maybe worth making that a bit more clear above ( there is a comment off to the side that says use null to run tests. It maybe more clear to have another line under SkipMessage with SkipMessage = null commented out and a comment above to uncomment to run skipped tests.

In reply to: 196930627 [](ancestors = 196930627,195885915)

Keeping this as it is as discussed offline

Can you please elaborate? We might need to refer to this PR in a future when we have all forgotten what was discussed offline :)

Scott said it is fine to have the current line.
private const string SkipMessage = "Fill in credentials before running test"; // Use null to run tests

I did -- so setting SkipMessage=null has the same effect as the SkipTestException since our version of XUnit does not support it. But I think the comment of "// Use null to run tests" could be a bit more explicit. This is more what I was thinking:

SkipMessage="....";
//Uncomment SkipMessage=null to run tests
//SkipMessage=null
``` #Resolved

Changed as suggested

Why does the uri need to trim the '/'? #Resolved

The ListBlobsSegmented does not work is there is a '/' at the beginining of the path. I have changed to AbsolutePath.TrimStart('/')

In reply to: 195886086 [](ancestors = 195886086)

ok thanks

In reply to: 196934429 [](ancestors = 196934429,195886086)

Why was the change reverted? #Resolved

Is there a better way to do this? Please see here:
https://hadoop.apache.org/docs/r2.4.1/hadoop-project-dist/hadoop-common/FileSystemShell.html#test
hdfs dfs -test -d will check to see if the path is a directory. #Resolved

I had tried to use this initially but it does not return any value. It sets the value in $? but I am not sure how to run that command using the commandrunner

In reply to: 195886573 [](ancestors = 195886573)

So $? is a linux variable that contains the value of the last run command. This will be 0 for success or 1 for failure. Talking with you offline, it looks like %errorlevel% in dos would work, however we need to account for linux as well. I think the right way to do this is to echo the %errorlevel% for now and file a JIRA to handle this correctly on linux. Parsing an integer from stdout should be a lot simpler and less error prone.

In reply to: 196918217 [](ancestors = 196918217,195886573)

fixed

In reply to: 197282264 [](ancestors = 197282264,196918217,195886573)

task.Wait can be removed and just have return task.Result

this has been taken care of in a previous PR but I have changed it here too

minor nit pick, this if statement could be reduced to return stdOut.Any() && stdOut.First() == '0';

fixed

nit: cleanup block to make more readable.

cleaned up

In reply to: 198553473 [](ancestors = 198553473)

use UrlPathSeparator constant

fixed

In reply to: 198668431 [](ancestors = 198668431)

not sure why we need maxBlobResults constant here. for readability, we can just write maxResults: 1, e.g.

var blobItems = _client.ListBlobsSegmented(
    path,
    useFlatListing: false,
    BlobListingDetails.Metadata,
    maxResults: 1,
    continuationToken: null,
    blobRequestOptions: null,
    operationContext: null).Results;

fixed

In reply to: 198669314 [](ancestors = 198669314)

maybe blobItems.OfType<CloudBlobDirectory>().Any()?

fixed

In reply to: 198670124 [](ancestors = 198670124)

parameter name prefix differs from the base name path

fixed

In reply to: 198671019 [](ancestors = 198671019)

we use task.Result in this method, but .GetAwaiter().GetResult() in ListDirectoryBlobsSegmented() implementation. is there are reason for it? if not, we should probably use same calls everywhere for consistency

changing all task.Result to task.GetAwaiter().GetResult() based on this discussion https://stackoverflow.com/questions/17284517/is-task-result-the-same-as-getawaiter-getresult which says task.Result hides actual exceptions

In reply to: 198671715 [](ancestors = 198671715)

Use more general return type, e.g. IList or even IEnumerable

Maybe use Linq? e.g.

private IEnumerable<CloudBlockBlob> CreateTempBlobs(string directory, int fileCount = 3)
{
    return Enumerable.Range(0, fileCount).Select(i =>
    {
        var filePath = directory + '/' + i;
        var blockBlob = _container.GetBlockBlobReference(filePath);
        UploadFromString(blockBlob, "hello");
        Assert.True(CheckBlobExists(blockBlob), "Blob does not exist: " + filePath);
        return blockBlob;
    });
}

changed

In reply to: 198681302 [](ancestors = 198681302)

We test for three different conditions here. Maybe split it into three unit tests?

done

In reply to: 198681751 [](ancestors = 198681751)

We test for several different conditions here. Maybe split this test into several independent tests?

fixed

In reply to: 198682001 [](ancestors = 198682001)

I would split that test into three independent tests - one for each condition

fixed

In reply to: 198682763 [](ancestors = 198682763)