Remove dependencies on external systems from unit tests

[kevingy]

The unit tests are failing again, at least on my machine. From my current location, I am unable to access afn.org, where some of the pages for referrer unit tests are hosted. Perhaps afn.org is down for everyone, world-wide?

In general, the unit tests are a bit of a mess. In addition to afn.org, there is a handful of other external systems over which the SimpleBrowserDotNet organization has no control whatsoever. If those systems go down or change, the unit tests break. That's not good process. The unit tests should only break if a code change in SimpleBrowser breaks them. False negatives are inefficient and erode confidence in the quality of the unit tests.

I see refactoring unit tests as being a long process. As a first step in cleaning up the unit tests, let's remove all external dependencies. The project should be self-contained, without the need for external systems. This will also assist in testing in a CI/CD environment.

Notes:

1. Currently, the unit tests are broken up into InternalTests, OnlineTests, and OfflineTests. This structure will no longer make sense if all of the test are now OfflineTests. A new structure will need to be determined and the project updated accordingly.
2. NUnit is currently in use. A good companion to it, which will probably be required is NSubstitute. In order to eliminate the external systems, a class or process that mimics a web server may be required. NSubstitute may be able to assist with that.

[kevingy]

Unit tests are building again. The files have been moved from afn.org to yenc-post.org

[kevingy]

Ugh. It's going to take even more work to refactor the unit tests than I thought, possibly leading to re-architecting the project. It doesn't look like there is any kind of a quick-fix.

There is very tight coupling between the Browser class and almost every other class in the project. (Bad!) With the current architecture, it's very difficult to unit test classes in the project individually, without testing Browser and other classes in the project at the same time. The way the code works right now, individual classes can't be isolated for unit testing.

The current unit tests demonstrate the problem. There is code in SimpleBrowser project that serves no purpose other than to support unit testing. That code is released to NuGet as a part of the "real" project. Specifically, the Browser class was modified to accept a constructor parameter (IWebRequestFactory) that determines if the Browser instance is a unit test Browser or a release Browser. Rather than refactoring the code to support better unit testing, a hack was introduced to make unit testing of tightly coupled code easier. (Fail!)

These need to be deleted:

• SimpleBrowser.Network.IWebRequestFactory
• SimpleBrowser.Network.DefaultRequestFactory
• SimpleBrowser.UnitTests.Helper
• Moq NuGet package in the UnitTest project (replaced with NSubstitute)
• The SampleDocs folder in the UnitTest project

Then, interfaces need to be created to support using dependency injection in the SimpleBrowser code and unit tests. (That is SimpleBrowser benefits from the code change, and it just so happens to allow for better and easier unit testing.)

Once all that is done, all of the unit tests will need to be modernized and converted to the triple-A pattern. Each unit test method should test exactly one thing. There are methods in the unit test project that test a dozen things in one method.

[kevingy]

I realize that many of the unit tests in the project were written long ago, but ... let's agree on some basic unit testing fundamentals in modern software engineering:

1. Each unit test tests exactly one thing. There shouldn't be ten Asserts at the end of a unit test to verify ten different things.
2. Unit test classes are named after the object they test. For example, unit tests for the Browser class are in a class (and file) named BrowserTests.
3. Unit test methods are named after the method they test using the pattern . For example to test the Browser constructor, the unit test method would be: Constructor_Invoked_CreatesObject()
4. Every code path needs a test - every if, every switch case, every exception. If there is code that can't be tested, an explanation of why needs to accompany it.
5. Moq is old and busted. Use nUnit and nSubstitute instead. It's cleaner, offers better control over tests, and allows us to remove code in the main project that's there only for unit testing.
6. Unit tests should test SimpleBrowser code ONLY! All external dependencies should be abstracted away by wrapping them with an interface and substituting that interface with nSubstitute.
7. Each unit test should implement the triple-A pattern:

[TestMethod]
public void Constructor_Invoked_CreatesObject()
{
   // Arrange

   // Act
   Browser browser = new Browser();

   // Assert
   Assert.IsNotNull(browser);
}

I'm going to start implementing this pattern. All pull requests after today will be required to:

• Update unit tests on the code they touch to match the above standard.
• Attempt 100% unit test code coverage on code modified and provide justification if code coverage is not 100%.

The person approving the pull request is required to verify compliance with these standards.