Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Common store test for LoadNextMsgMulti #5275

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

neilalexander
Copy link
Member

It concerns me a little that by duplicating the same test for both file and memory stores, that differences could slip in later that mean we are no longer testing for consistency between the two stores.

This approach allows us to write tests that only use the StreamStore API and have them tested against all store permutations.

Signed-off-by: Neil Twigg [email protected]

@neilalexander neilalexander requested a review from a team as a code owner April 4, 2024 11:17
@neilalexander
Copy link
Member Author

Example:

=== RUN   TestStoreMsgLoadNextMsgMulti
=== RUN   TestStoreMsgLoadNextMsgMulti/Memory
=== RUN   TestStoreMsgLoadNextMsgMulti/File
=== RUN   TestStoreMsgLoadNextMsgMulti/File/None-None
=== RUN   TestStoreMsgLoadNextMsgMulti/File/None-S2
=== RUN   TestStoreMsgLoadNextMsgMulti/File/AES-GCM-None
=== RUN   TestStoreMsgLoadNextMsgMulti/File/AES-GCM-S2
=== RUN   TestStoreMsgLoadNextMsgMulti/File/ChaCha20-Poly1305-None
=== RUN   TestStoreMsgLoadNextMsgMulti/File/ChaCha20-Poly1305-S2
--- PASS: TestStoreMsgLoadNextMsgMulti (0.65s)
    --- PASS: TestStoreMsgLoadNextMsgMulti/Memory (0.00s)
    --- PASS: TestStoreMsgLoadNextMsgMulti/File (0.65s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/None-None (0.11s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/None-S2 (0.11s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/AES-GCM-None (0.11s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/AES-GCM-S2 (0.11s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/ChaCha20-Poly1305-None (0.11s)
        --- PASS: TestStoreMsgLoadNextMsgMulti/File/ChaCha20-Poly1305-S2 (0.11s)
PASS
ok      github.com/nats-io/nats-server/v2/server

@neilalexander neilalexander force-pushed the neil/multi-filter-consumers branch from a6803bc to ead85a7 Compare April 4, 2024 12:20
Base automatically changed from multi-filter-consumers to main April 4, 2024 14:57
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am generally in favor.

A couple of points though.

  1. For tests that really have nothing to do with all permutations this seems quite wasteful and we do have some of those tests.
  2. Make sure to update Travis config so that these are properly run on travis. Ivan just made some changes to peel out the filestore tests, so we need to update that.

It concerns me a little that by duplicating the same test for both
file and memory stores, that differences could slip in later that
mean we are no longer testing for consistency between the two stores.

This approach allows us to write tests that only use the `StreamStore`
API and have them tested against all store permutations.

Signed-off-by: Neil Twigg <[email protected]>
@neilalexander neilalexander force-pushed the neil/multi-filter-consumers branch from ead85a7 to 1433ee3 Compare April 4, 2024 15:27
@neilalexander
Copy link
Member Author

Good point, I added a bool to testAllStoreAllPermutations to control whether or not to test all of the various compression and encryption options. When false, it now just tests the two stores:

=== RUN   TestStoreMsgLoadNextMsgMulti
=== RUN   TestStoreMsgLoadNextMsgMulti/Memory
=== RUN   TestStoreMsgLoadNextMsgMulti/File
--- PASS: TestStoreMsgLoadNextMsgMulti (0.01s)
    --- PASS: TestStoreMsgLoadNextMsgMulti/Memory (0.00s)
    --- PASS: TestStoreMsgLoadNextMsgMulti/File (0.01s)
PASS
ok      github.com/nats-io/nats-server/v2/server

I already copied in the same build tags that Ivan added to filestore_test.go last week so that should be fine.

@derekcollison derekcollison self-requested a review April 4, 2024 15:50
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@derekcollison derekcollison merged commit 0277a95 into main Apr 4, 2024
4 checks passed
@derekcollison derekcollison deleted the neil/multi-filter-consumers branch April 4, 2024 17:09
@kozlovic
Copy link
Member

kozlovic commented Apr 4, 2024

@neilalexander But you then need to add a go test -v -race -run=TestStore ... to the runTestsOnTravis.sh script (and GitHub action yaml file for good measure) because otherwise they will not be running. You can see that this new test did not run in any of the Travis runs.

derekcollison added a commit that referenced this pull request Apr 4, 2024
I missed this change in #5275.

Signed-off-by: Neil Twigg <[email protected]>
@derekcollison
Copy link
Member

Thanks @kozlovic, that was what I was worried about.

derekcollison added a commit that referenced this pull request Apr 4, 2024
Cherry-pick the following PRs into the v2.10.14 release branch:

* #5237
* #5238
* #5242
* #5208
* #5244
* #5248
* #5246
* #5112
* #5247
* #5256
* #5258
* #5264
* #5266
* #5267
* #5265
* #5271
* #5270
* #5272
* #5276
* #5274
* #5275
* #5277
* #5279

Signed-off-by: Neil Twigg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants