3
\$\begingroup\$

I'm working on a class to parse RSS feeds using SyndicationFeed. (related to Downloading data using HttpClient). I'm trying to write this so it can be unit-tested, but there's an awful lot of boring setup boilerplate before I get to the important stuff.

My data source sometimes sends invalid RSS XML according to XmlReader (in the pattern of <description>This article ... <mark>keyword</mark> ...</description>), so I need to sanitize the input (for political reasons, I can't just say "this won't work until they fix their end").

I started with the following method, which does a few too many things and isn't very testable. My important logic starts after the SyndicationFeed.Load() call, but any tests require that all the XML handling works correctly.

public virtual FeedSummary ParseResponse(string data)
{
    var response = new FeedSummary();

    var doc = new XmlDocument();
    doc.LoadXml(data);

    var builder = new StringBuilder();
    var xmlSettings = new XmlWriterSettings();
    xmlSettings.OmitXmlDeclaration = true;
    xmlSettings.ConformanceLevel = ConformanceLevel.Fragment;
    var htmlWriter = XmlWriter.Create(builder, xmlSettings);

    var transformationDocument = new XmlDocument();
    transformationDocument.LoadXml(this.XsltSource);
    //The xslt source can be passed in via the class constructor so 
    //I can test this using the identity transform
    var compiledTransform = new XslCompiledTransform(true);
    compiledTransform.Load(transformationDocument);
    compiledTransform.Transform(doc, htmlWriter);

    var reader = XmlReader.Create(new StringReader(builder.ToString()));
    var feed = SyndicationFeed.Load(reader);
    reader.Close();

    var totalResults = feed.ElementExtensions.Single(x => x.OuterName == "totalResults");
    response.TotalResults = Int32.Parse(totalResults.ToString());
    foreach (var item in feed.Items)
    {
        var hits = new List<SearchHit>();
        var searchItem = new SearchHit()
        {
            Title = item.Title.Text,
            ItemUrl = item.Links[0].Uri,
            Description = item.Summary.Text
        };
        hits.Add(searchItem);
        response.TopHits = hits;
    }

    return response;
}

I did try breaking it up into multiple methods, coming together in this one:

public FeedSummary ParseResponse(string rssFeed)
{
    var doc = LoadXml(rssFeed);
    var cleanData = SanitizeXml(doc);
    var feed = CreateFeed(cleanData);
    return ParseFeed(feed);
}

This is better than the first one in that each step can be tested independently, but my top-level method still has the same testability problems. I'd appreciate any thoughts on improving code organization, especially with respect to improving testability.

\$\endgroup\$
  • 2
    \$\begingroup\$ Where are your tests? The whole question seems to revolve around them, but they're not here. \$\endgroup\$ – RubberDuck Jul 23 '15 at 22:42
  • 2
    \$\begingroup\$ I guess that isn't very clear. I'm concerned about restructuring this to make it easily testable, not about the tests themselves. \$\endgroup\$ – Brian Jul 24 '15 at 0:23
2
\$\begingroup\$

By breaking the code into separate methods, you've made it easier to read, which is good. You've not made it easier to test though (unless you expose those methods to the test framework, which then creates a leaky abstraction and brittle tests). Therefore all you should be testing with your revised code is ParseResponse itself. ParseResponse has lots of side-effects though, so is hard to test.

The solution is to decouple those side-effect causing parts of the code and to inject them:

public FeedSummary ParseResponse(string rssFeed,
                                 IXMLLoader loader,
                                 IXMLSanitizer sanitizer,
                                 IFeedCreator feedCreator)
{
    var doc = loader.LoadXml(rssFeed);
    var cleanData = sanitizer.SanitizeXml(doc);
    var feed = feedCreator.CreateFeed(cleanData);
    return ParseFeed(feed);
}

The you can create implementations of IXMLLoader etc that can be individually tested with integration tests and provide mocks for testing ParseResponse itself.

\$\endgroup\$
  • \$\begingroup\$ This is perfect! Exactly what I needed to change in my code. \$\endgroup\$ – Brian Jul 24 '15 at 14:55

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.