[DISCUSS] Builder dedicated for testing

[Zili Chen <wa...@gmail.com>]

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.

Re: [DISCUSS] Builder dedicated for testing

[Till Rohrmann <tr...@apache.org>]

+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 <sh...@gmail.com>
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 <wa...@gmail.com> 于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.
> >
>

Re: [DISCUSS] Builder dedicated for testing

[SHI Xiaogang <sh...@gmail.com>]

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 <wa...@gmail.com> 于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.
>