Here is the PR I was referring to. https://github.com/apache/beam/pull/6205/files
On Wed, Aug 15, 2018 at 5:14 PM Alex Amato <[email protected]> wrote: > Currently, we do not have any related test files, i.e. operations_test.py > or bundle_processor_test.py. I > > I looked into adding an operations_test.py, but its very difficult to > instantiate the Operations classes and mock out the parts I need in order > to test that monitoring_infos are populated properly. > > For instance, in the base Operation class the spec is difficult to mock > class Operation: > def __init__(self, name_context, spec, counter_factory, state_sampler): > > I could just mock out everything until I get it to work. but I think that > would lead to a difficult to maintain test and a complicated mocking > pattern, mocking several deep layers in the spec. > > Right now, I have added some testing code which tests this via the > MetricResult.query() layer, using fn_api_runner_test.py > But I would like to setup tests which test the returned monitoring_infos > in bundle_processor.py > > I was wondering what your thoughts are on moving forward. In order to have > more testing in these intermediate layers, I think we need to do some > refactoring. > The options I see are: > > 1. Make several test helpers that can instantiate the various > operations classes. Maybe refactor out a lot of our creation/factory code > so that it can be used in tests. > 1. Ideally we would want something > 2. Continue with the compex mocking approach, but try to use the same > code across several tests, to try and make it easier to maintain (However, > if we change the code much, we will have to spend a lot of time fixing up > the mocks which need to have a strict adherence to things several layers > deep.) > 3. Redesign these layers to need minimal parameters, and allow for > easier instantiation of each class . This might be hard to revise at this > point. > 4. To finish up my PR add a private testing layer to access the > bundle_processor's monitoring_info's method. I'll probably do this to move > forward with the PR. > 1. This approach tests the composite system as a group. This > approach is fine if its tested extensively. More of a black box style, > >
