4
\$\begingroup\$

I am new to writing unit tests and i am facing some difficult writing unit tests for this class. I am using Xunit and moq.

public class WriteFileOutput: IWriteFileOutput
{
    public string File { get; set; }
    public WriteFileOutput(string fileName)
    {
        File = fileName;
    }
   
    public void writeFile(List<CustomerDistanceRecord> invitees)
    {
        var result = JsonConvert.SerializeObject(invitees, Formatting.Indented);
        if (System.IO.File.Exists(File))
        {
            System.IO.File.Delete(File);
            using(var tw = new StreamWriter(File, true))
            {
                tw.WriteLine(result.ToString());
                tw.Close();
            }
        }
        else if (!System.IO.File.Exists(File))
        {
            using(var tw = new StreamWriter(File, true))
            {
                tw.WriteLine(result.ToString());
                tw.Close();
            }
        }
    }
}

The code below tests that the writefileoutput writes out a file. I am not sure if this is a good unit test and I was wondering if someone could show me a better way of testing it class. Also I am not sure it deleting the file in the constructor before it is called is a good idea either. Thanks for the help.

 private readonly string _path = "../../../TestOutput/CustomerRecords.json";

    public WriteFileTests()
    {
        if (File.Exists(_path))
        {
            File.Delete(_path);
        }
    }

    [Fact]
    public void WriteFileOuput_should_be_called()
    {
        var fileWriteFileOutput = new WriteFileOutput($"{_path}");
        fileWriteFileOutput.writeFile(It.IsAny<List<CustomerDistanceRecord>>());
        Assert.True(File.Exists($"{_path}"));
    }
\$\endgroup\$
3
  • 1
    \$\begingroup\$ IMHO I don't see the point of testing built-in functionality. If you can't trust the framework... \$\endgroup\$
    – BCdotWEB
    Jun 13 '20 at 23:01
  • \$\begingroup\$ This is an integration test that is targeting implementation concerns. the subject class is also tightly coupled to implementation details. \$\endgroup\$
    – Nkosi
    Jun 14 '20 at 0:00
  • \$\begingroup\$ So you are saying it is tighly coupled . How do I make it loosely coupled. Have a method that checks if the file exists and return a bool ? \$\endgroup\$
    – user226060
    Jun 14 '20 at 9:57
4
\$\begingroup\$

IMHO there is nothing in this class that requires integration testing. At most you should be able to mock it, but all of its functionality is calling methods that are presented by the framework (or by a NuGet package, e.g. the JSON serialization). If you feel the need to test those, then why not write a test to check that the return value of some method that returns a string is actually a string and not some other type of object?

You could argue that perhaps it might fail to create a file because it needs to do so on a share and that share is inaccessible, but then it is perfectly possible that this happens on production and not a single integration test will catch it. That's why exceptions exist and logging etc.

And that's where your code really fails: at no point do you apply a try...catch and do you log any possible IO exceptions. Sure, maybe your exception will bubble up and get caught elsewhere and reported, but you need to be sure about that. For instance: it is entirely possible an existing file cannot be deleted because another process has it locked.

Perhaps you should catch any IO exceptions here and return a bool to report back whether the file was written without issues (if you care about that, if other code depends on the file existing,...).


Your WriteFileOutput class is also pretty bad in other ways. Its name sounds like a method and is fairly undescriptive anyway. Moreover, you've obviously copy-pasted code instead of thinking through its logic. This does exactly the same while using far less lines and has no duplication:

    if (System.IO.File.Exists(File))
    {
        System.IO.File.Delete(File);
    }

    using(var tw = new StreamWriter(File, true))
    {
       tw.WriteLine(result.ToString());
       tw.Close();
    }

Some quick remarks:

  • What is even the point of else if (!System.IO.File.Exists(File))? What else could the else to if (System.IO.File.Exists(File)) even be?

  • public void writeFile doesn't follow the standards WRT naming (methods should be PascalCase).

  • Why is this public: public string File { get; set; }? Why can it be set from outside?

  • Both File and fileName are incorrect names. You're clearly passing a path to a file, not the name of a file (and "File" is even less descriptive).

  • result is too generic a name.

  • Are you saving other objects in much the same way as List<CustomerDistanceRecord> invitees? If so, do you have a specialized class for each of those? Because that seems overkill to me when the logic of writeFile (except for var result = JsonConvert.SerializeObject(invitees, Formatting.Indented);) can easily be reused.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the comments. The reason why I wanted to test it is because I want high code coverage. I was using public string File {get;set} because i was setting the value using DI property injection. I changed the logic of the code to have a datetimestamp in it so that I dont need to delete the file. \$\endgroup\$
    – user226060
    Jun 15 '20 at 13:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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