Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

Conversation

@afchung
Copy link
Contributor

@afchung afchung commented May 11, 2016

…acilitate testing for REEF-1357

JIRA:
REEF-1389

@dkm2110
Copy link
Contributor

dkm2110 commented May 12, 2016

@afchung I am wondering what are the relation of these interfaces to the IFileSystem interface we already have. In some sense shouldn't IDirectoryInfo and IFileInfo be part of general IFileSystem interface and we just need to expand it...Or I am over generalizing? Basically, if a user comes to REEF, and looks at these interfaces will he be clear what IFileSystem is intended for and what IFileInfo and other related interfaces intended for. I will do the review once we figure out this question. @markusweimer can you also comment?

@afchung
Copy link
Contributor Author

afchung commented May 12, 2016

@dkm2110 these interfaces are simply to allow us to perform tests by mocking out DirectoryInfo and FileInfo. These, at the moment, very specifically refer to local directories and files only, and many of these concepts/methods may not apply to remote file systems. We can in time certainly extend them for usage in IFileSystem, but supporting such interfaces will not be trivial for remote file systems.


public DateTime LastAccessTimeUtc
{
get { return _directoryInfo.LastAccessTime; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_directoryInfo.LastAccessTimeUtc?

@dkm2110
Copy link
Contributor

dkm2110 commented May 13, 2016

@afchung Left some minor comments. Apart from that two more points:
a) There is no documentation currently for any public functions in interface and implementations.
b) If we think that these interfaces and implementations will be used only in tests in future we can move it to test project also.

@afchung
Copy link
Contributor Author

afchung commented May 13, 2016

@dkm2110 for b), these interfaces and implementations will not only be used in tests, but will be used to facilitate testing. For that to work, we need to integrate it into our public facing APIs.

For example when the user wants to use an IFileInfo object in our APIs, they will simply do:
IFileInfo info = DefaultFileInfo.FromFileInfo(new FileInfo(...));

For our testing, because we don't actually want to use a FileInfo object, we can use NSubstitute in the following way:

IFileInfo infoForTesting = Substitute.For<IFileInfo>();
infoForTesting.DirectoryName.Returns("FakeDirectoryName");

Such that when we test one of our functions with the parameter of an IFileInfo with the fake infoForTesting object, it will not perform actual actions on a file. This requires our functions to expose an interface instead of a class, since NSubstitute does not work on concrete classes without virtual methods such as FileInfo01.

@afchung
Copy link
Contributor Author

afchung commented May 13, 2016

@dkm2110 I've addressed your comments, please have another look. Thanks!

@dkm2110
Copy link
Contributor

dkm2110 commented May 14, 2016

@afchung basically I learnt something new i.e. Nsubstitute :) ... I will test and merge later tonight.

@afchung
Copy link
Contributor Author

afchung commented May 16, 2016

@dkm2110 what is the status on this?

@dkm2110
Copy link
Contributor

dkm2110 commented May 16, 2016

Testing now.

/// Factory method to create an <see cref="IDirectoryInfo"/> object
/// from a <see cref="DirectoryInfo"/> object.
/// </summary>
public static IDirectoryInfo FromDirectoryInfo(DirectoryInfo info)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is DirectoryInfo defined? is this a standard C# class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is a standard C# class.

@markusweimer
Copy link
Contributor

I'm also confused about the role of these new classes. Do they add interfaces on top of APIs we already have? If so, all uses of the classes should be replaced with uses of the interfaces.

@afchung
Copy link
Contributor Author

afchung commented May 16, 2016

@markusweimer They do not add interfaces on top of APIs we already have. They are a precursor to allow #994 to be unit-testable. For more explanation, see my reply do @dkm2110 above. Thanks!

@markusweimer
Copy link
Contributor

Is the following a fair description of this change?

This adds interface for classes in the .NET standard library such that we can mock implementations of them for our unit tests.

dkm2110 pushed a commit to dkm2110/reef that referenced this pull request May 16, 2016
@afchung
Copy link
Contributor Author

afchung commented May 17, 2016

@markusweimer yes, should I reflect that message somewhere?

@asfgit asfgit closed this in cce59bb May 17, 2016
@markusweimer
Copy link
Contributor

I've copied this description to the JIRA. In case we ever wonder again :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants