+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 <shixiaoga...@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 <wander4...@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. > > >
