Store tests don't really test the stores

[alinnert]

I've implemented a BadgerStore (code is already on GitHub as part of a demo project) to support Badger and I was about to implement the tests so I could create a PR. But then I've noticed that the current tests actually don't test the stores themselves but the API they are using. Like: redisstore_test.go doesn't test redisstore.go but redigo instead. This should not be part of scs but of redigo.

With the exception of memstore these tests don't seem to be very useful to me.

Since the functionality of all stores is basically the same, wouldn't it make sense to write one test that tests the basic functionality of scs with every store? From the perspective of scs the outcome should always be the same, no matter what store you use. So, if scs works as expected while using store x then store x also has to work as expected. Right? Also, this should make developing new stores a lot easier, since you don't have to worry about writing tests.

What do you think about that?

[alexedwards]

Hi,

A PR containing a store for Badger would be great, thank you!

The way things are currently handled, the basic behaviour of SCS is tested in the session_test.go and data_test.go files, and the implementation of the store is tested by redisstore_test.go etc.

I would argue that the store tests do have some use. The redisstore_test.go isn't just testing that redigo works, it is also a sanity check that the store code is calling the correct Redis commands in the correct order to achieve a specific outcome. In the case of the SQL-based stores, the tests sanity check that the correct SQL queries have been written, and also that background 'cleanup' goroutine to delete expired sessions is working correctly.

But in principle your suggestion is an interesting one, and I agree that "if scs works as expected while using store x then store x also has to work as expected".

I'm not sure about how to do that in practice though. I wouldn't want to run the battery of behaviour tests for each store from the main scs package, because that would mean pulling a load of additional dependencies into the main scs package.

Do you have any alternative ideas?

[alinnert]

the tests sanity check that the correct SQL queries have been written, and also that background 'cleanup' goroutine to delete expired sessions is working correctly.

This is true, but it does so by duplicating the implementation code. You have to make sure both are in sync and do the same thing. I think it's better if instead the tests create a store and call its methods and check if they work. That way the code can't get out of sync. And it's more similar for all stores.

However, I think there are several ways to tackle this. One could just call the store's methods (Commmit, Find, Delete, Find) (method 1a). Or call the methods and check the results directly by accessing the storage engine behind the store (method 1b). I wonder, is this necessary, though? Maybe if the store provides a function like NewWithPrefix to check if it really uses that prefix. Thinking about this, individual store tests might indeed be useful.

When it comes to global testing, one could create a function TestStores, which creates one scs instance for every store and runs tests on those instances (probably in parallel) (method 2). Or is this overkill when the tests I've described above are implemented as well?

Now, I actually prefer method 1b.

[alexedwards]

I think it's better if instead the tests create a store and call its methods and check if they work... Or call the methods and check the results directly by accessing the storage engine behind the store (method 1b)

I'm a bit confused, sorry!

As far as I can see, what you've described there is what the tests are doing. As an example, consider the postgres.TestSaveNew test. I've added extra some comments to annotate it:

func TestSaveNew(t *testing.T) {
        // 1. Reset the test database to a known state before starting the test.
    dsn := os.Getenv("SCS_POSTGRES_TEST_DSN")
    db, err := sql.Open("postgres", dsn)
    if err != nil {
        t.Fatal(err)
    }
    defer db.Close()
    if err = db.Ping(); err != nil {
        t.Fatal(err)
    }
    _, err = db.Exec("TRUNCATE TABLE sessions")
    if err != nil {
        t.Fatal(err)
    }

        // 2. Initialize a new instance of the store.
    p := NewWithCleanupInterval(db, 0)

        // 3. Call the store's Commit() method.
    err = p.Commit("session_token", []byte("encoded_data"), time.Now().Add(time.Minute))
    if err != nil {
        t.Fatal(err)
    }

        // 4. Access the storage engine behind the store directly to check that the Commit() 
        // method did what it was meant to (i.e. inserted a row with the expected session         
        // token and payload).
    row := db.QueryRow("SELECT data FROM sessions WHERE token = 'session_token'")
    var data []byte
    err = row.Scan(&data)
    if err != nil {
        t.Fatal(err)
    }
    if reflect.DeepEqual(data, []byte("encoded_data")) == false {
        t.Fatalf("got %v: expected %v", data, []byte("encoded_data"))
    }
}

This is true, but it does so by duplicating the implementation code.

Using the same example, the implementation code for the postgres Commit() method looks like this:

func (p *PostgresStore) Commit(token string, b []byte, expiry time.Time) error {
    _, err := p.db.Exec("INSERT INTO sessions (token, data, expiry) VALUES ($1, $2, $3) ON CONFLICT (token) DO UPDATE SET data = EXCLUDED.data, expiry = EXCLUDED.expiry", token, b, expiry)
    if err != nil {
        return err
    }
    return nil
}

That logic (or anything like it) doesn't appear in the test cases. The logic in the test cases is simply related to setting up the test database to a known state before starting the test, and checking the result of calling Commit, Delete, Find directly afterwards (if applicable).

Sorry if I've misunderstood your point. If I have, could you try rewording or post a pseudo-code example of what you mean by 1b : )

[alinnert]

You did not misunderstand. I just noticed my mistake. By translating from redis/redigo code to badger I misinterpreted the New(redisPool). I thought it would create a new redis pool or something, which would be irrelevant for my tests. I mean, there's new() after all, which would do just that. If that was the case, there would be no RedisStore instance that would be tested in the tests.

Anyway... Once I've finished the tests I will create a PR. 😅

[alexedwards]

I see, no worries!

Are you OK for me to close this?

[alinnert]

Sure, you can close this.