engine/test: Rework StorageEngine's test utils #1494

a-savchuk · 2024-11-13T11:08:59Z

a-savchuk commented

2024-11-13 11:08:59 +00:00

Remove testNewShard and setInitializedShards because they violated the default engine workflow. The correct workflow is: first use New(), followed by Open(), and then Init(). As a result, adding new logic to (*StorageEngine).Init caused several tests to fail with a panic when attempting to access uninitialized resources. Now, all engines created with the test utils must be
initialized manually. The new helper method prepare can be used for that purpose.
Additionally, setInitializedShards hardcoded the shard worker pool size, which prevented it from being configured in tests and benchmarks. This has been fixed as well.
Ensure engine initialization is done wherever it was missing.
Refactor setShardsNumOpts, setShardsNumAdditionalOpts, and setShardsNum. Make them all depend on setShardsNumOpts.

Close #1491 - Remove `testNewShard` and `setInitializedShards` because they violated the default engine workflow. The correct workflow is: first use `New()`, followed by `Open()`, and then `Init()`. As a result, adding new logic to `(*StorageEngine).Init` caused several tests to fail with a panic when attempting to access uninitialized resources. Now, all engines created with the test utils must be initialized manually. The new helper method `prepare` can be used for that purpose. - Additionally, `setInitializedShards` hardcoded the shard worker pool size, which prevented it from being configured in tests and benchmarks. This has been fixed as well. - Ensure engine initialization is done wherever it was missing. - Refactor `setShardsNumOpts`, `setShardsNumAdditionalOpts`, and `setShardsNum`. Make them all depend on `setShardsNumOpts`.

a-savchuk added 3 commits 2024-11-13 11:09:00 +00:00

[#1491 ] engine/test: Use more suitable testing utils here and there 61b8124d71

Use `setShardsNum` instead of `setInitializedShards` wherever possible.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

[#1491 ] engine/test: Move BenchmarkExists to exists_test.go 93ed775e9c

Move `BenchmarkExists` from `engine_test.go` to `exists_test.go`
for better organization and clarity.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

[#1491 ] engine/test: Rework engine test utils

DCO action / DCO (pull_request) Successful in 1m47s

Details

Tests and linters / Run gofumpt (pull_request) Successful in 2m12s

Details

Vulncheck / Vulncheck (pull_request) Successful in 3m5s

Details

Tests and linters / Staticcheck (pull_request) Successful in 3m40s

Details

Build / Build Components (pull_request) Successful in 3m54s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 3m49s

Details

Tests and linters / Lint (pull_request) Successful in 4m26s

Details

Tests and linters / gopls check (pull_request) Successful in 4m42s

Details

Tests and linters / Tests (pull_request) Successful in 6m32s

Details

Tests and linters / Tests with -race (pull_request) Successful in 7m23s

Details

b97db5f285

- Remove `testNewShard` and `setInitializedShards`. They violated
the default engine workflow, which should be: first use `New()`,
followed by `Open()`, and then `Init()`. As a result, adding new
logic to `(*StorageEngine).Init` caused several tests to fail with
a panic when attempting to access uninitialized resources. Now all
engines created with test utils must be initialized manually.
- Also, `setInitializedShards` hardcoded the shard worker pool size,
preventing it from being configured in tests/benchmarks Fix it too.
- Ensure engine initialization is done wherever it's missing.
- Refactor `setShardsNumOpts`, `setShardsNumAdditionalOpts`, and
`setShardsNum`. Make them all depend on `setShardsNumOpts`.

Signed-off-by: Aleksey Savchuk <a.savchuk@yadro.com>

a-savchuk force-pushed prettify-engine-test-utils from b97db5f285 to bd772c4da3

2024-11-13 11:09:41 +00:00

Compare

fyrchik reviewed 2024-11-13 11:12:23 +00:00

pkg/local_object_storage/engine/head_test.go Outdated

					
				@ -43,1 +42,3 @@

						s2 := testNewShard(t)

						te := testNewEngine(t).setShardsNum(t, 2)

						e := te.engine

						require.NoError(t, e.Open(context.Background()))

fyrchik commented

2024-11-13 11:12:21 +00:00

What about making init() method on the test engine?
The point of test helpers is to reduce boilerplate, it is much shorter to use testNewEngine(t).setShardsNum(t, 2).init() than te := testNewEngine(t).setShardsNum(t, 2) + 2 more lines.

What about making `init()` method on the test engine? The point of test helpers is to reduce boilerplate, it is much shorter to use `testNewEngine(t).setShardsNum(t, 2).init()` than `te := testNewEngine(t).setShardsNum(t, 2)` + 2 more lines.

👍 1