5

I have a unit test which is intended to check the access right and also make sure there is enough access right to file system before constructing a class. But, I think my unit test is violating the unit testing principle (FIRST) such as: Isolated/Independent and Thorough which are talking about building an independent unit test and also considering several aspects of failure scenarios. I also believe that this unit test in the way that is implemented is unit testing the .Net framework not the functionality of the code written by me because the underlying class is not mocked. But I would like to know your opinions as well and see how wrong or right I am.

Explanation of the Unit Test:

The unit test is trying to make sure that the constructor of EventFeedFile is executing a method which is checking the access rights. That method called VerifyWritable(). Note that the EventFeedFile is a separate class and the VerifyWritable() is seating in another class.

[Test]
public void DirectoryNotWritableTest()
{
    var tempPath = Path.Combine(Path.GetTempPath(), "EventFileDirectoryWriteDeniedTest");
    int pageSize = 10;

    try
    {
        if (Directory.Exists(tempPath))
            Directory.Delete(tempPath, true);

        var directoryInfo = Directory.CreateDirectory(tempPath);
        var securityRules = directoryInfo.GetAccessControl();
        var writeDeniedRule =
            new FileSystemAccessRule("Everyone", FileSystemRights.CreateFiles, AccessControlType.Deny);
        securityRules.AddAccessRule(writeDeniedRule);
        directoryInfo.SetAccessControl(securityRules);

        // ReSharper disable once ObjectCreationAsStatement
        Assert.Throws<UnauthorizedAccessException>(() => new EventFeedFile(new PersistenceDirectory(tempPath),
            pageSize));
    }
    finally
    {
        if (Directory.Exists(tempPath))
            Directory.Delete(tempPath, true);
    }
}

/// <param name="directory">The directory containing the persistence file.</param>
/// <param name="pageSize">The size of event feed page.</param>
public EventFeedFile(PersistenceDirectory directory, int pageSize)
{
    directory.EnsureExists();
    directory.VerifyWritable();
    mParentDirectory = directory.TargetPath;
    mFullName = Path.Combine(mParentDirectory, Constants.FILE_NAME_EVENTPERSISTENCE);
    mPageSize = pageSize;
}

public void VerifyWritable()
{
    using (File.Create(
        Path.Combine(TargetPath, Path.GetRandomFileName()),
        1024,
        FileOptions.DeleteOnClose))
    {
    }
}
5
  • 1
    It depends entirely on how you wrap your mind around the problem. In the end, all unit-tests could be considered testing the framework or the compiler. However, the question you need to ask yourself. Is this unit test testing that a specific class behaves according to contract? If the class has been documented and specified that if you construct the EventFeedFile on top of a directory that has been locked down, you should get an UnauthorizedAccessException, then the test is OK. Commented Oct 12, 2018 at 11:24
  • Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.
    – Nkosi
    Commented Oct 12, 2018 at 11:33
  • either you have write access or you don't... so i think this should be refined/refactored. why is pageSize passed into the function, i would say that the logic of checking security type stuff is in the wrong place. put in a helper with static method called HasWriteAccessToDirectory, which you pass the directory, the intention is gd just the usage is fishy/ wrong place.
    – Seabizkit
    Commented Oct 12, 2018 at 11:33
  • 2
    To be wrong here is the name of this test which is poor in every aspect. It seems to imply that you're testing directory write permissions. Hopefully the intent was to assert that given a not writable directory the ctor will throw a specific exception. That's a. Perfectly reasonable test. Commented Oct 12, 2018 at 11:36
  • Some good comments and answers on this questions overall, SO = winning
    – Seabizkit
    Commented Oct 12, 2018 at 11:43

3 Answers 3

5

As you said, You are testing EventFeedFile
Also you said VerifyWritable is in a different module.

Then it is pretty obvious VerifyWritable() should be mocked to return or throw respectively to the test intention.
instead of doing Two bad things:

  1. Testing the VerifyWritable Code when you didn't want to.
    in case it is faulty, your test will fail on out of scope code (or worse, will pass on faulty code).

  2. You are actually Writing files, and Access rules which for my opinion bad practice, will scale badly, and might be changing when switching FileSystems/OperatingSystems (or versions)

Note: The test doesn't actually tests the .NET Framework... it just utilizing the Real Environment, instead of creating a Mocked (Virtual environment) for the Test use.

2
  • I believe my comment above on answer and this answer go hand in hand. i'm pretty sure we saying the same thing? your is just better if it is.
    – Seabizkit
    Commented Oct 12, 2018 at 11:36
  • If the underlying code for checking the security was unit testing independently (as a separate module) with some custom business logic then yes I would do agree with you but since it is being tested through another class and does not provide any business logic then it is more likely unit testing the .Net framework. Commented Oct 13, 2018 at 14:17
2

I don't think there's a clear yes/no answer here.

A unit test should not access external data like a file system. But okay, at some point you have to test it and if the means to do that automatically is a unit testing project in VS then so be it. Leave the naming to the purists, I'm happy with automated tests.

Yes, you are right, the code under test is plain .NET Framework. So yes, it's testing the framework. But it's also testing if somebody actually used the framework to do this job. If the requirement is to throw that exception on construction and not later in the process, then this is a valid test. If someone messes with the code and introduces an error, this test will find it.

So I'd say this is a good test. It's better to have it then to delete it. It makes sure the requirement of checking the accessibility is implemented correctly.

1
  • I clearly understand that the reason for having that unit test is to make sure that the VerifyWritable method is being executed before finalizing the class construction. But, there is two problems in here: 1- That access check will be necessary on the write/read time, who knows between creating the class and starting to use the write/read method how would look like that access rights. 2- The VerifyWritable method is from another class which should be mocked in the class of EventFeedFile. Commented Oct 12, 2018 at 11:42
0

Thanks for all of you for providing nice opinions. To complete, I also would like to say my own opinion this test is suffering from several problems. which I will try to short listed in here:

  1. That access write check will be necessary on the writing/reading time, who knows between creating the class and starting to use the write/read method how would look like that access rights. Plus, what if after constructing the class access rights has changed for any reason.
  2. The VerifyWritable method is from another class which should be mocked in the class of EventFeedFile.
  3. What if we do not check for any access right? In the end it will fail due to UnauthorizedAccessException and since we are not wrapping up this exception with another application exception or taking any specific action like logging right-way etc. the test be comes useless. (This is why I am saying this is unit testing the .Net framework.)
  4. From the functionality point of view checking for the access right only on the constructing time is not sufficient and also the way of checking access right is so vague
1
  • is it a system check or does the directly actually change? You should rename and re-position to reflect its intention
    – Seabizkit
    Commented Oct 12, 2018 at 11:59

Not the answer you're looking for? Browse other questions tagged or ask your own question.