+1 for trying to avoid cluttering production code with testing code.
Whenever possible we should add a testing utility to fulfill our testing
requirements instead of adding the code to the production class.
Cheers,
Till
On Tue, Aug 27, 2019 at 11:07 AM SHI Xiaogang <
[hidden email]>
wrote:
> Hi Tison,
>
> Thanks for bringing this up to discussion.
>
> I think it's helpful to reducing unnecessary constructors with instance
> builders in test scope.
> Now certain classes, e.g., Execution, ExecutionVertex and StateHandle, are
> instantiated (including mocking and spying) here and there in the test
> code, with different arguments.
> I'd like to cleanup related code with introduced Builders.
>
> Regards,
> Xiaogang
>
> Zili Chen <
[hidden email]> 于2019年8月26日周一 下午9:21写道：
>
> > Hi devs,
> >
> > I'd like to share an observation that we have too many
> > @VisibleForTesting constructors that only used in test scope such as
> > ExecutionGraph and RestClusterClient.
> >
> > It would be helpful if we introduce Builders in test scope for build
> > such instance and remain the production code only necessary
> > constructors.
> >
> > Otherwise, code becomes in mess and contributors might be confused by
> > a series constructors but some of them are just for testing. Note that
> > @VisibleForTesting doesn't mean a method is *only* for testing.
> >
> > Best,
> > tison.
> >
>
Locked
