Update some of the QueueManagerTests to use Storage instead

[tomwilkie]

To catch bugs like #3561

[vaibhavsingh97]

Hi! I just learnt go and want to take up this issue 😄 Also, please share necessary resources which will help me fixing this issue quickly. 😅

[tomwilkie]

Happy to review a PR! The PR linked above has some more discussion, and the relevant tests are here: https://github.com/prometheus/prometheus/blob/master/storage/remote/queue_manager_test.go

[gouthamve]

@tomwilkie I am trying to help @vaibhavsingh97 with this and am a little lost myself! So from what I see, none of the tests use Storage or any of its mocks.

Do you want to use the actual Storage and then test the right parameters were passed down? While we could do that, that seems like testing implementation, can we somehow test the behavior?

I lack context around the remote APIs, so forgive me if the answer to this question is a little obvious :P

[tomwilkie]

Do you want to use the actual Storage

Yes

and then test the right parameters were passed down?

Not really - I'm thinking in a test like TestSampleDelivery instead of using NewQueueManager, one could use remote.NewStorage and do ApplyConfig instead. That would catch the aforementioned bug as these tests rely on the setting specific fields in the config to illicit specific behaviour - in this case a single shard to trigger completely in-order delivery.

Of course, its not that easy as the APIs are different, but its should be too challenging.

[bege13mot]

Hi @tomwilkie,

I would like to help with this issue, but also a little lost myself as a gouthamve. Could you please clarify some details regarding the implementation?

TestSampleDelivery use TestStorageClient but ApplyConfig using StorageClient created as NewClient(i, &ClientConfig .... So I could create new test method similar ApplyConfig that will support TestStorageClient or change initial ApplyConfig method. But it seems to be the wrong direction. Could you please advise me about it?

Initially, we would like to test that QueueConfig will be applied during storage.ApplyConfig is it correct?