Improve BaseTestSuite #56
Conversation
rihter007
force-pushed
the
refactor/improve_runner_base_test_suite
branch
from
d3c90a0
to
cb45196
Compare
| type BaseTestSuite struct { | ||
| suite.Suite | ||
|
|
||
| pluginRegistry *pluginregistry.PluginRegistry | ||
| internalStorage *MemoryStorageEngine | ||
| Ctx xcontext.Context |
There was a problem hiding this comment.
there's very clear guidelines on not keeping contexts in structs
Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx:
instead this should be created (and cancelled) on a per test basis
There was a problem hiding this comment.
I agree with this guidelines in general, but this is a TestSuite which is some sort of global environment for each test.
It is handy because:
- I use context during TestSetup() (which can't be passed from the test itself, so I will have two context in the test)
- Tests do not get simple, but yet redundant logic of context initialisation with logs support + cancelling it (I agree that it is short, but still it is code duplication).
There was a problem hiding this comment.
Ha-ha, noticed that the first point is affected by NewPluginRegistry that doesn't follow that guideline
There was a problem hiding this comment.
Indeed I can get rid of context in MemoryStorageEngine
There was a problem hiding this comment.
this usage here s.PluginRegistry = pluginregistry.NewPluginRegistry(s.Ctx) should not exist to begin with. What's being used there in terms of semantics is a logger, not a cancellation context. I'll accept the review as is right now, but please post an issue to get that fixed and then remove the context from the struct
There was a problem hiding this comment.
Opened a new issue: #59
rihter007
force-pushed
the
refactor/improve_runner_base_test_suite
branch
from
cb45196
to
37fde4b
Compare
Use BaseTestSuite in TestRunner tests Signed-off-by: Ilya <[email protected]>
rihter007
force-pushed
the
refactor/improve_runner_base_test_suite
branch
from
37fde4b
to
09ece8d
Compare
|
Wtf, I have a strong feeling that I removed ctx from BaseSuite... |
Move global variable ctx into suite
Use BaseTestSuite in TestRunner tests