1:      public class Advent15
   2:      {
   3:          private FileUtilWithDelete SetUpReadable(string content)
   4:          {
   5:              FileUtilWithDelete file = new FileUtilWithDelete("SomeFile.txt");
   6:              file.Create(content);
   7:              return file;
   8:          }
   9:   
  10:          private FileUtilWithDelete SetUpUnreadable(string content)
  11:          {
  12:              FileUtilWithDelete file = SetUpReadable(content);
  13:              file.Readable = false;
  14:              return file;
  15:          }
  16:   
  17:          [Fact]
  18:          public void TestReadReadableFile()
  19:          {
  20:              using (FileUtilWithDelete file = SetUpReadable("CONTENT"))
  21:              {
  22:                  string content = file.Read();
  23:                  Assert.Equal<string>("CONTENT", content);
  24:              }
  25:          }
  26:   
  27:          [Fact]
  28:          public void TestReadUnreadableFile()
  29:          {
  30:              using (FileUtilWithDelete file = SetUpUnreadable("SHOULD NOT BE ABLE TO READ THIS"))
  31:              {
  32:                  Assert.Throws<AccessViolationException>(() => { file.Read(); });
  33:              }
  34:          }
  35:      }

The two setup methods does not really feel necessary. Why not add another parameter to it and control readability that way?

   1:      public class Advent14
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUpReadable(string content)
   6:          {
   7:              m_file = new FileUtilWithDelete(Path.GetTempFileName());
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          private void SetUpUnreadable(string content)
  12:          {
  13:              SetUpReadable(content);
  14:              m_file.Readable = false;
  15:          }
  16:   
  17:          [Fact]
  18:          public void TestReadReadableFile()
  19:          {
  20:              SetUpReadable("CONTENT");
  21:              string content = m_file.Read();
  22:              Assert.Equal<string>("CONTENT", content);
  23:          }
  24:   
  25:          [Fact]
  26:          public void TestReadUnreadableFile()
  27:          {
  28:              SetUpUnreadable("SHOULD NOT BE ABLE TO READ THIS");
  29:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  30:          }
  31:      }

The use of the new FileUtilWithDelete-class makes the tests neater than before I think. But the member variable should also be removed since it is a legacy from the time when we used the unit test framework setup-method. And we need to create a unique file name in order to get rid of the Dispose method. The use of a temporary (unknown) name makes it hard to find the file if something goes wrong and it is left on the host. So I want to get rid of that as soon as possible too.

   1:      public class Advent13 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUpReadable(string content)
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          private void SetUpUnreadable(string content)
  12:          {
  13:              SetUpReadable(content);
  14:              m_file.Readable = false;
  15:          }
  16:   
  17:          public void Dispose()
  18:          {
  19:              m_file.Delete();
  20:          }
  21:   
  22:          [Fact]
  23:          public void TestReadReadableFile()
  24:          {
  25:              SetUpReadable("CONTENT");
  26:              string content = m_file.Read();
  27:              Assert.Equal<string>("CONTENT", content);
  28:          }
  29:   
  30:          [Fact]
  31:          public void TestReadUnreadableFile()
  32:          {
  33:              SetUpUnreadable("SHOULD NOT BE ABLE TO READ THIS");
  34:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  35:          }
  36:      }

So far we've used the unit test framework clean up mechanism to remove the test file after each test. I don't see anything wrong with that but another way to do it would be to introduce a new class that does this for us. Tomorrow I'll make use of this new class:

    public class FileUtilWithDelete : FileUtil, IDisposable
    {
        public FileUtilWithDelete(string path)
            : base(path)
        {
            
        }

        public void Dispose()
        {
            Delete();
        }
    }

   1:      public class Advent12 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUp(string content)
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadReadableFile()
  18:          {
  19:              SetUp("CONTENT");
  20:              string content = m_file.Read();
  21:              Assert.Equal<string>("CONTENT", content);
  22:          }
  23:   
  24:          [Fact]
  25:          public void TestReadUnreadableFile()
  26:          {
  27:              SetUp("SHOULD NOT BE ABLE TO READ THIS");
  28:              m_file.Readable = false;
  29:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  30:          }
  31:      }

Since the setup-method is called explicitly, why not have several setup-methods for different kinds of setup?

   1:      public class Advent11 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUp(string content)
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadOK()
  18:          {
  19:              SetUp("CONTENT");
  20:              string content = m_file.Read();
  21:              Assert.Equal<string>("CONTENT", content);
  22:          }
  23:   
  24:          [Fact]
  25:          public void TestReadFails()
  26:          {
  27:              SetUp("SHOULD NOT BE ABLE TO READ THIS");
  28:              m_file.Readable = false;
  29:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  30:          }
  31:      }

OK, now let's take a look at the test names again. TestReadOK is actually a pretty decent name since it says "Reading works". TestReadFails is not a good name since it does not say anything about why the read operation is expected to fail. Next refactoring step will just be a rename of the test methods.

   1:      public class Advent10 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUp(string content)
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadOK()
  18:          {
  19:              SetUp("CONTENT");
  20:              string content = "";
  21:              Assert.DoesNotThrow(() => { content = m_file.Read(); });
  22:              Assert.Equal<string>("CONTENT", content);
  23:          }
  24:   
  25:          [Fact]
  26:          public void TestReadFails()
  27:          {
  28:              SetUp("SHOULD NOT BE ABLE TO READ THIS");
  29:              m_file.Readable = false;
  30:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  31:          }
  32:      }

Some people think a good rule of thumb is to have only one assert in each test. I think it is OK to have more if the assert adds value if an unexpected failure happens. In this case I think line 21 does not really add value. If an exception is thrown the framework will report the unexpected exception anyway so let's get rid of that one.

   1:      public class Advent9 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUp(string content)
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create(content);
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadOK()
  18:          {
  19:              SetUp("CONTENT");
  20:              Assert.DoesNotThrow(() => { m_file.Read(); });
  21:          }
  22:   
  23:          [Fact]
  24:          public void TestReadFails()
  25:          {
  26:              SetUp("SHOULD NOT BE ABLE TO READ THIS");
  27:              m_file.Readable = false;
  28:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  29:          }
  30:      }

So now that I got a setup-method that that accepts desired content I should probably use it to actually verify the content of the file that reads OK...

   1:      public class Advent8 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          private void SetUp()
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create("CONTENT");
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadOK()
  18:          {
  19:              SetUp();
  20:              Assert.DoesNotThrow(() => { m_file.Read(); });
  21:          }
  22:   
  23:          [Fact]
  24:          public void TestReadFails()
  25:          {
  26:              SetUp();
  27:              m_file.Readable = false;
  28:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  29:          }
  30:      }

Unfortunately I've reintroduced the fact that the actual content is not verified for TestReadOK. And using a constant isn't something I want to do again. But since I'm calling the setup method explicitly I can modify it to accept the desired content. Wouldn't that be neat?

   1:      public class Advent7 : IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:          private const string m_content = "CONTENT";
   5:   
   6:          public Advent7()
   7:          {
   8:              m_file = new FileUtil("SomeFile.txt");
   9:              m_file.Create(m_content);
  10:          }
  11:   
  12:          public void Dispose()
  13:          {
  14:              m_file.Delete();
  15:          }
  16:   
  17:          [Fact]
  18:          public void TestReadOK()
  19:          {
  20:              string content = "";
  21:              Assert.DoesNotThrow(() => { content = m_file.Read(); });
  22:              Assert.Equal<string>(m_content, content);
  23:          }
  24:   
  25:          [Fact]
  26:          public void TestReadFails()
  27:          {
  28:              m_file.Readable = false;
  29:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  30:          }
  31:      }

Having the expected content in a constant member is pretty convenient but if the class grows and gets a lot of tests I'll have to scroll quite a bit to see the actual value. Personally I think you should have all the information needed for the test local in the test. Also I'm not a big fan of using the unit test framework "setup" mechanism since the reader of the test must know that the framework executes a certain setup-method. Even though that is probably true most of the time I think it is better to explicitly call the setup method and make it visible exactly how the test is set up.

   1:      public class Advent6: IDisposable
   2:      {
   3:          private IFileUtil m_file;
   4:   
   5:          public Advent6()
   6:          {
   7:              m_file = new FileUtil("SomeFile.txt");
   8:              m_file.Create("CONTENT");
   9:          }
  10:   
  11:          public void Dispose()
  12:          {
  13:              m_file.Delete();
  14:          }
  15:   
  16:          [Fact]
  17:          public void TestReadOK()
  18:          {
  19:              Assert.DoesNotThrow(() => { m_file.Read(); });
  20:          }
  21:   
  22:          [Fact]
  23:          public void TestReadFails()
  24:          {
  25:              m_file.Readable = false;
  26:              Assert.Throws<AccessViolationException>(() => { m_file.Read(); });
  27:          }
  28:      }

With some redundant code broken out into the common setup method (the default constructor if you're not familiar with Xunit.net) there is another thing that bothers me about the TestReadOK-test. We're not actually verifying the content of the file. Let's fix that in the next version of this test.