engine/test: Rework `StorageEngine`'s test utils #1491

New issue

Closed

opened 2024-11-13 07:12:46 +00:00 by a-savchuk · 0 comments

a-savchuk commented

2024-11-13 07:12:46 +00:00

Member

Block solutions for #1450

Problems

Minor problem

(*testEngineWrapper).setInitializedShards mostly duplicates (*StorageEngine).addShard and hardcodes the shard worker pool size, preventing it from being configured in tests/benchmarks

	`func (te testEngineWrapper) setInitializedShards(t testing.TB, shards ...shard.Shard) *testEngineWrapper {`
	`for _, s := range shards {`
	`pool, err := ants.NewPool(10, ants.WithNonblocking(true))`

Major problem

Some utils indirectly use testNewShard function, which itself initializes a new shard. This violates 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 causes several tests to fail with a panic when attempting to access uninitialized resources

Solution

Since, the only purpose of (*testEngineWrapper).setInitializedShards is to create shards before creating an engine, allowing future access them in the future, it's unnecessary to initialize the shard before initializing the engine. I suggest the following changes:

remove shard initialization from testNewShard
return a slice of created shards along with the engine when creating a test engine
ensure engine initialization is done wherever it's missing

Block solutions for #1450 ### Problems #### Minor problem [`(*testEngineWrapper).setInitializedShards`](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/f1556e3c42764499c2a7f4f9c75f8b13a7d643d7/pkg/local_object_storage/engine/engine_test.go#L91-L107) mostly duplicates [`(*StorageEngine).addShard`](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/f1556e3c42764499c2a7f4f9c75f8b13a7d643d7/pkg/local_object_storage/engine/shards.go#L177-L202) and hardcodes the shard worker pool size, preventing it from being configured in tests/benchmarks https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/f1556e3c42764499c2a7f4f9c75f8b13a7d643d7/pkg/local_object_storage/engine/engine_test.go#L91-L93 #### Major problem Some utils indirectly use [`testNewShard`](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/f1556e3c42764499c2a7f4f9c75f8b13a7d643d7/pkg/local_object_storage/engine/engine_test.go#L190-L201) function, which itself initializes a new shard. This violates 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` causes several tests to fail with a panic when attempting to access uninitialized resources ### Solution Since, the only purpose of `(*testEngineWrapper).setInitializedShards` is to create shards before creating an engine, allowing future access them in the future, it's unnecessary to initialize the shard before initializing the engine. I suggest the following changes: - remove shard initialization from [`testNewShard`](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/f1556e3c42764499c2a7f4f9c75f8b13a7d643d7/pkg/local_object_storage/engine/engine_test.go#L190-L201) - return a slice of created shards along with the engine when creating a test engine - ensure engine initialization is done wherever it's missing