Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am writing automated functional tests for my application. To interact with the application, I use the SQL database to retrieve data and validate data.

One of the challenges that I'm facing is that some data that I need to pull at run time can take 5 minutes to execute a SQL query (specifically this query GetAccessionNumber()), significantly impacting the performance of my tests, especially when most of the tests need to do this query. Also, I foresee that the query in GetAccessionNumber might change at run time.

Although I am currently talking to the DB to retrieve my data, I wonder if there is a good design pattern or strategy for such a situation to improve my test code. Also, I want to do this efficiently so that I don't waste 5 minutes every test just reading data. Maybe executing all the queries at Setup() and use all the data for my tests and then TearDown() after?

Someone suggested populating the data that I need into like a "test db" and then using that through my tests, rather than reading the entire db (which also contains irrelevant data for my purposes). How would you do that? Let's say I have about 30 queries that I need to run every single build to get the latest test data. Should I run that before all of my tests through Setup() method? Or should I run these queries externally, populate my own testing table, and then let the tests use this new "test db"?

I am adding a working sample of the code to better convey my problems.

        [Test]
    [Retry(2)]
    [Description("Validate that the pop up can open and close when it is clicked.")]
    public void ToggleOpenAndClose()
    {
        var home = new HomePage(Driver);
        home.GoTo();
        home.Login();

        var adminReset = new AdminResetPage(Driver);
        adminReset.GoTo();

        //todo not sure if this is the right implementation
        adminReset.SetLoginId(TestData.TestData.LoginId);
        adminReset.SetBookletTo(TestData.TestData.AccessionNumber);
        var assPage = adminReset.GoToBookletLocation();

        assPage.ShortTextPopup.ClickPopUp();
        assPage.ShortTextPopup.ClosePopUp();
        Assert.IsFalse(assPage.ShortTextPopup.PopupIsOpen(), "The pop up did not close after trying to close the pop up");
    }




public static class TestData
{
    private static Logger _logger = LogManager.GetCurrentClassLogger();
    static readonly string _connectionString = string.Format(
        "Data Source=.\\foobar_2017;" +
        "Initial Catalog={0};" +
        "User ID={1};" +
        "Password={2};", 
        ConfigurationManager.AppSettings["DB.DataBaseName"],
        ConfigurationManager.AppSettings["DB.UserId"],
        ConfigurationManager.AppSettings["DB.Password"]);
    //todo temporarily hardcoded until we implement the sql reader
    public static string LoginId
    {
        get { return "P2abc"; }
    }

    public static string AccessionNumber
    {
        get { return "AC123"; }
        //todo temp hardcoded for debugging
        //get { return GetAccessionNumber(_connectionString); }
    }

    public static User GetUser(UserType userType)
    {
        if (Config.IsEnvironmentLocalHost())
            return GetLocalhostUser(userType);

        if (Config.IsEnvironmentDev())
            return GetDevUser(userType);

        return GetTestUser(userType);
    }

    private static User GetTestUser(UserType userType)
    {
        switch (userType)
        {
            case UserType.Admin:
                return new User() { Password = "myPass", UserName = "myUserName" };
            default:
                throw new ArgumentOutOfRangeException(nameof(userType), userType, null);
        }
    }

    private static User GetDevUser(UserType userType)
    {
        switch (userType)
        {
            case UserType.Admin:
                return new User() { Password = "myPass2", UserName = "myUserName2" };
            default:
                throw new ArgumentOutOfRangeException(nameof(userType), userType, null);
        }
    }

    private static User GetLocalhostUser(UserType userType)
    {
        switch (userType)
        {
            case UserType.Admin:
                return new User() {Password = "admin$", UserName = "password$"};
            default:
                throw new ArgumentOutOfRangeException(nameof(userType), userType, null);
        }
    }

    private static void GetStudentLoginId(String connectionString)
    {
        using (SqlConnection connection = new SqlConnection(connectionString))
        {
            SqlCommand command = new SqlCommand(
              "SELECT CategoryID, CategoryName FROM Categories;",
              connection);
            connection.Open();

            SqlDataReader reader = command.ExecuteReader();

            // Call Read before accessing data.
            while (reader.Read())
            {
                //ReadSingleRow((IDataRecord)reader);
            }
            reader.Close();
        }
    }

    private static string GetAccessionNumber(string connectionString)
    {
        List<string> accessionNumbers = new List<string>();

        using (SqlConnection connection = new SqlConnection(connectionString))
        {
            var query = "select AccessionNumber from Item where " +
                        "ContentHtml like '%short-text-popup%' " +
                        "and AccessionNumber not like '%help%' ";
            //"ContentHtml like '%short-text-popup%'" +
            //"and AccessionNumber not like '%help%' ";
            SqlCommand command = new SqlCommand(query, connection);
            command.CommandTimeout = 300;
            connection.Open();

            SqlDataReader reader = command.ExecuteReader();

            // Call Read before accessing data.
            while (reader.Read())
            {
                accessionNumbers.Add(reader["AccessionNumber"].ToString());
            }
            reader.Close();
        }


        return accessionNumbers[0];
    }
}
share|improve this question

put on hold as off-topic by forsvarir, t3chb0t, Tunaki, Malachi, Marc-Andre yesterday

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

1  
I couldn't verify whether it did what you want it to do because it's only part of the code involved. The line which made me think it's broken is Basically, I am wondering if there is some kind of a design pattern that I should be using to make sure that I properly talk to the database for the test data. Can you clarify whether you're currently talking properly to the database with your test data? If so, I'll retract my close vote. If not, you're asking for something that isn't implemented yet. That's what we mean with 'code not yet written'. – Mast yesterday
1  
It looks to me that the code works, but the question is still difficult to answer. I have a hard time believing that loading TestData is taking 5 minutes, as you say in the question. Is there something else that you haven't shown us? – 200_success yesterday
    
Thanks @mast i updated the question to better reflect that it is currently working code. Just not optimized as I believe that there is a better solution. Or not, depending on the communities advice – Nikolay Advolodkin yesterday
    
@200_success it is this query in that method - GetAccessionNumber() . The DB is large in size and I finding this data takes a long time. I have no control of the DB or the data inside. – Nikolay Advolodkin yesterday
    
I've voted to close this question as 'stub' code. In its current form. GetStudentLoginId for example just reads a bunch of rows an ignores the result.. the main query that is the issue GetAccessionNumber isn't actually called etc. – forsvarir yesterday

Only Read What You Need

So, the first thing I noticed was that your GetAccessNumber method is fetching all of the rows returned by the database. Since you're doing a pretty board full text search on two columns, my guess is that it's returning a fair number of rows. The method itself is then throwing all but the first record away. If you don't need the rest, don't fetch them. Changing the query to only retrieve the first row might help:

var query = "select TOP 1 AccessionNumber from Item where " +
                    "ContentHtml like '%short-text-popup%' " +
                    "and AccessionNumber not like '%help%' ";

Cache input data that's reused by your tests

The tests you've posted are incomplete, and so it's hard to know what you're actually planning on doing. However, your commented out code suggests that you're planning on calling getAccessNumber every time AccessNumber is required:

public static string AccessionNumber
{
    get { return "AC123"; }
    //todo temp hardcoded for debugging
    //get { return GetAccessionNumber(_connectionString); }
}

Your _connectionString is static, readonly, so presumably never changes. Are you expecting that GetAccessNumber should return a new value for every call? If not, then why not just save the value on the first call (or read the value immediately through a static constructor) and simply return it on subsequent calls?

share|improve this answer
    
Those are all excellent suggestions. Let me further clarify. In regards the query, doing select top 1 won't do anything because i only get back about 10 rows. Your latter suggestion, the GetAccessionNumber will return different results based on the query that I use in there. Right now it's focused on 1 query, but I foresee how I will need to be able to update that query based on the needs of my test. For example, I may need "contentHtml like checkbox" for 1 test or I may need "contentHtml like multipleChoice" for another and so on. Different tests will require their own AccessionNumber. – Nikolay Advolodkin yesterday
    
@NikolayAdvolodkin Have you actually run the query with TOP 1 to compare the execution time? Whilst you're only retrieving 10 rows, in order for the database to determine that there are only 10 it is probably performing a full table scan. If it knows you're only interested in the first result, it can abort the scan as soon as it finds one. – forsvarir yesterday

I'm concerned about your comment:

I have no control of the DB or the data inside.

If this is true then your tests are unreliable. You should create a test database and populate it with test data so your queries don't run forever and you can actually repeat the test and get the same results for the same data. Currently your tests just prove that your tests run.

Besides this query:

var query = "select AccessionNumber from Item where " +
                        "ContentHtml like '%short-text-popup%' " +
                        "and AccessionNumber not like '%help%' ";

looks like it should belong to some repository selecting the data that also should be tested.

If you don't select data like this then why are you doing it in the tests? If so then definitely it should be tested as a part of a repository or something.

share|improve this answer
    
You make some great points. One thing I'm unclear about, why do you say that my tests are unreliable because I don't have control of the DB? But your suggestion of having a test DB seems good, I was thinking about the same thing, but wasn't sure if that would be the correct approach here. How would you do that? Let's say I have about 30 queries that I need to run every single build to get the latest test data. Should I run that before all of my tests? Or should I run these queries externally and populate my own testing table and then let the tests use this new "test db"? – Nikolay Advolodkin yesterday
    
I used a similar approach as described on Dynamically creating a LocalDb database in tests but modified for entity framework Database Initialization Strategies in Code-First. Just think what happens to your (production?) database when your tests don't pass and delete or modify the half of it, it would be a disaster (unless you have ready-only permisions). – t3chb0t yesterday

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