Proof of concept of using TestStepInputOutput that hides using channels behind interface. #144
Conversation
…ls behind interface. Previously each TestStep operated on targets using channels inside TestStepChannels. Which introduced an ability to close output channel as well as it didn't allow to override behaviour when TestStep obtains a new Target. Signed-off-by: Ilya <rihter007@inbox.ru>
| } | ||
|
|
||
| type targetInput struct { | ||
| tgt target.Target |
There was a problem hiding this comment.
it might actually be better to have the copy here, but does subtly change semantics
There was a problem hiding this comment.
That is true, but why not to start getting rid of pointers?
There was a problem hiding this comment.
But ok, I agree that it is better to do it in a separate PR
| ctx.Debugf("paused during target acquisition, acquired %d", len(targets)) | ||
| return nil, xcontext.ErrPaused | ||
|
|
||
| cancel() |
There was a problem hiding this comment.
from my reading of xcontext.WithCancel, the errs array is used to notify waiters with it, when cancel() is called.
so if the ctx here is already signalled with ErrPaused, it doesnt make sense to notify again with ErrPaused.
maybe im wrong in reading the xcontext code. the point is that prev code returned ErrPaused in this case, and this now returns stepIO.Get() error, which isnt equivalent.
There was a problem hiding this comment.
so if the ctx here is already signalled with ErrPaused, it doesnt make sense to notify again with ErrPaused.
Indeed, there is no need to notify about the same thing twice. In xcontext: if something happened at least once, it means it already happened (and this is forever) and repeating a signal won't affect anything. This behavior seems to be consistent with how original cancel signal works in the standard context.
| onConsumed func() | ||
| } | ||
|
|
||
| func (tsi *testStepInputOutput) Get(ctx xcontext.Context) (*target.Target, error) { |
There was a problem hiding this comment.
this api might also allow for those "get some", "get all" semantics that appear in some plugins
There was a problem hiding this comment.
type TestStepInputOutput interface {
Get(ctx xcontext.Context) (*target.Target, error)
GetMany(ctx xcontext.Context) ([]target.Target, error)
?
BUT
the main difference is in ctx.
In the first case we wait for either first target or context
In the second case we accumulate targets until they either deplete or context will Done() (for example timeout).
|
i had a look over the code, but i need more time to think of the implications since this would be breaking api. |
Previously each TestStep operated on targets using channels inside TestStepChannels.
Which introduced an ability to close output channel as well as it didn't allow to override behaviour when TestStep obtains a new Target.
Tests might break. It is just a PoC