Add tests for the volume filter (part of #483)

[debnil]

This PR adds tests for the volume filter. This lets us add tests for the volume filter, which in turn lets us make changes needed for buyTwap (part of #522 and #548).

This PR does not close issue #483 since we've split up the tests into two parts.

[nikhilsaraf]

I haven't checked the values on TestVolumeFilterFn yet since't it's a little hard to go through.

I've made some comments on how we can have a simpler interface to these test cases, which should make it a lot easier to go through.

I've added quite a few comments, let me know if you want to discuss tomorrow to help unblock.

[nikhilsaraf]

shouldn't this function also take in the action sell or buy?
this way we can easily test the two actions (buy/sell) rather than hard-coding "sell" in the production code in a hidden function. It's not super clear in the tests or production code that we are using the sell action because it's not hidden away in this function.

If we really need this hidden functionality then the function should be named as such to include the "sell" logic, such as: makeDailyVolumeByDateQueryForSellAction

[nikhilsaraf]

on second thought -- not sure if it makes a lot of sense to extract this function out.

I see we're using it in the test, but what this function is really doing is testing the MakeMarketID method and the utils.Dedupe method.

Wouldn't it be better to call queries.MakeDailyVolumeByDateForMarketIdsAction in the test directly?

[debnil]

Questions all make sense. My reasoning: the extraction was to generate needed queries for test cases. This avoids repeating the same 5-6 lines, when we vary the market IDs or account IDs. I didn't add the action to sell or buy in this PR because there's no notion of "buy" in the codebase yet - it would be added in the subsequent PR that tests buy out.

With that said, I don't think this matters if we constrain the test cases, as suggested below, and reduce database calls.

[nikhilsaraf]

we should have a helper function called makeManageSellOffer(price, volume) so we don't have to create structs everywhere -- makes it a lot more readable

[nikhilsaraf]

better yet -- this should be a parameter to the testCase struct input and the test should construct the appropriate manage sell offer from these two fields of price and volume

[nikhilsaraf]

similarly, the inputs for otb and tbb should be:

• otbBaseCap
• otbQuoteCap
• tbbBaseCap
• tbbQuoteCap

this will make these test case structs a lot smaller and will contain less boilerplate

[nikhilsaraf]

we have a price of 1.0 in a few places -- that's not very helpful since we won't do any price conversions then. It should be anything but 1.0 :)

[nikhilsaraf]

I haven't checked the values on these tests yet but once we have a simpler interface to these test cases it will be a lot easier to go through. we should end up without any struct constructions in tests cases wherever possible.

[nikhilsaraf]

may be good to discuss over a call today, lmk

[nikhilsaraf]

it seems that we are using this function in the "want" part of the tests. For that use case, this is not the correct implementation of the function.

Let me explain the idea here -- we want to test that the "function under test" (in this case the factory method) is transforming the given inputs to the correct outputs. The outputs for such unit tests should almost always have very hard-coded values as the outputs. Why this seems like an incorrect usage is because in this function we are running through "MakeMarketID" and "utils.Dedupe", which means:

1. it's not entirely obvious from the test case exactly what we will end up with for the marketIDs and accountIDs in the underlying DailyVolumeByDate
2. we are repeating the logic in the "function under test" in the "want" part of the test. this will introduce unnecessary overhead in maintaining the code since we now need to modify two places to make a change in the factory method and we're not getting much additional value from the second implementation.

I think the intention was probably to "easily construct the query with an easier API interface in the want part of the test" to keep the test cases more readable, which is a very good thing to do. imo, the best approach to do that is to have a very transparent function that is not doing anything more than directly creating an object from the passed in params:

func makeWantDailyVolumeQuery(marketIDs []string, action string, optionalAccountIDs []string) *queries.DailyVolumeByDate {
    return queries.MakeDailyVolumeByDateForMarketIdsAction(&sql.DB{}, marketIDs, action, optionalAccountIDs)
}

• we don't have any transformations happening to the want values that we see directly in the test case
• I've called this makeWant.. so it's clear that it's used in the "want" part of the test code. Since this is test code and the test case clearly labels the type of query, we can also abbreviate the function name to makeWantQuery to keep it even shorter
• we get the convenience of not constructing a struct in every testCase while keeping the name short and automatic db creation, so the usage and readability is much better
• note, we can also just use queries.MakeDailyVolumeByDateForMarketIdsAction(&sql.DB{}, marketIDs, action, optionalAccountIDs) directly, but having a small wrapper function is a little better for readability.

PS: let's avoid params in function signatures like this: (name1, name2 int) and instead use a more explicit approach like this: (name1 int, name2 int). It maintains the convention of clear types for each variable so you don't have to "lookahead" a few variables to figure out the type of a given variable (increases readability).

[nikhilsaraf]

Note: when calling this new function, we will now need to input the "marketID" for the market into the marketIDs string array that we pass in, which is more explicit and better so the reader of the test knows exactly what is the end query being constructed.

[nikhilsaraf]

if you want to have two types of these functions, one as makeWantSellQuery and makeWantBuyQuery, that's ok with me too.

[debnil]

Cool, this mostly makes sense - thanks for the lengthy explanation, super helpful :)

I think I'm just going to eliminate this function. Other refactors around helpers mean that this logic can be explicitly pushed up into makeTestVolumeFilter. Those changes are pushed, let me know how it looks.

[nikhilsaraf]

name and base/quote assets are not changing across the test cases -- can we move these to be inside the test?

we also don't need to re-specify the config inside the wantSubmitFilter since that's exactly the same as what we pass in -- in the test we can check that assert.Equal(t, k.config, submitFilter.config)
in fact I think the test case should only take in 1 want value: wantDailyVolumeByDateQuery.
we don't need wantError either because none of our cases will produce an error

Take a look at TestFilterOrdersByVolume for an example of a test where the testCases does not match the function inputs and outputs exactly to make them more readable and reduce boilerplate (link)

[nikhilsaraf]

additionally -- we can have two test runs for each case specified here inside the t.Run() function below -- one with nil base cap, and the other with a nil quote cap. that way we don't need to specify 2x cases here.

we can do the same for exact and ignore -- since those aren't modified either. so this will be a nested for loop for each case specified here.

something like this:

for test cases:
    for _, m := range []volumeFilterMode{exact, ignore} {
        baseCapConfig := &VolumeFilterConfig{
            SellBaseAssetCapInBaseUnits: point.Float64(1.0),
            SellBaseAssetCapInQuoteUnits: nil,
            mode: m,
            additionalMarketIDs: k.additionalMarketIDs,
            optionalAccountIDs: k.optionalAccountIDs,
        }

        quoteCapConfig := &VolumeFilterConfig{
            SellBaseAssetCapInBaseUnits: nil,
            SellBaseAssetCapInQuoteUnits: point.Float64(1.0),
            mode: m,
            additionalMarketIDs: k.additionalMarketIDs,
            optionalAccountIDs: k.optionalAccountIDs,
        }

        for _, c := range []*VolumeFilterConfig{baseCapConfig, quoteCapConfig} {
            isBaseCap := c.SellBaseAssetCapInBaseUnits != nil
            name := fmt.Sprintf("%s_%s_%v", k.namePrefix, m.String(), isBaseCap)
            t.Run(name, func(t *testing.T) {
                // refer to TestFilterOrdersByVolume for how to convert test case inputs to function inputs and function outputs to test case outputs
            })
        }
    }

does this make sense?

[nikhilsaraf]

this will reduce the test cases significantly

for the empty config, we can have a special test since it doesn't conform to this model

[nikhilsaraf]

let's hold off on changes to TestVolumeFilterFn, may be better to move it to a separate PR since it's hard to follow and many of the comments that apply to the structure of the test above apply here. I think that what we learn from an improved structure to the first test will make writing and reviewing this test much easier -- this test is also a lot more complex because it's the main functionality and deals with numbers.

[debnil]

I made some further changes that mostly simplified the interface but also rewrote VolumeFilterFn as a standalone function rather than a method. If you still want to hold off on changes, can do so, but might be good to just get it done in one swoop since it's massively simplified.

[nikhilsaraf]

usage of this for want value should be something like (pseudocode):

makewantvalueFilter("somemarketID", marketIDsArray, accountIDsArray)

what is clearly missing is the "someMarketID" string in the want part of the test, that's a clear red flag

[debnil]

This refactor makes enough sense, I've pulled out the "someMarketID" string from this function and am defining it in the test.

[nikhilsaraf]

test cases should take a want variable. the inputs should be structured so the want values are different

[debnil]

We could do this, but it's a bit messy. Within our current test structure, we'd have to define the testCases within the inner loop of our array for this to work.

The want variable requires a volumeFilterConfig. In a prior comment, you suggested changing the wantFilter method to take config as parameter - that code change has been made. So, our current approach defines the testCases and then loops over configs and modes. This forecloses referencing the config in the testCases array - the config doesn't exist at this point - so we cannot construct the wantVolumeFilter in the tests as of now.

[nikhilsaraf]

it's ok to create the test cases inside the for loop in this situation

[nikhilsaraf]

I think this negative value sentinel makes the code a lot more complex.
we now need to keep in our minds the idea of an alternative representation (instruction) of how to create a volume filter config.

we also only use it in ~5 places. not sure i understand the benefit of this -- I had thought that we agreed to remove these sentinel values but I may be mistaken

[debnil]

I don't think we decided either way. Definitely agreed that it's not amazing, but we don't have a great alternative yet, and I'm totally open to another way we can vary this.

[nikhilsaraf]

I think it's ok to use pointers (via pointy) directly instead of the -1 sentinels. I think this will help readability.

[nikhilsaraf]

I don't see a want value defined here -- where is the expected output for this test?

[nikhilsaraf]

it's ok to create the test cases inside the for loop in this situation

[nikhilsaraf]

delete line 40 (see below)

[nikhilsaraf]

exchange name should be part of the input -- we don't need the exchangeName variable -- if we use a common variable for the inputs then we're not really changing the inputs to the function

want marketID should be part of the input (if we are constructing the wantVolumeFilter in the test function)

[nikhilsaraf]

wantMarketIDs should be []string{"marketID1", "marketID2"},

[nikhilsaraf]

wantMarketIDs should be a non-empty string array here I think (would be the string inlined value in firstMarketID)?

[nikhilsaraf]

reviewing later once we have the factory method well tested -- I think we should split these up into separate PRs

[nikhilsaraf]

should be good to go once we remove the unused function commented inline

[nikhilsaraf]

lgtm, thanks! 🚢

[nikhilsaraf]

Note: This PR does not close issue #483 since we've split up the tests into two parts (fyi @debnil)
we should close the gh issue when we add the test for the function as well