Thanks for getting attention to this topic. We relied on ValidatesRunner tests and encapsulating tests which is not optimum. For the PR, approach 4 seems to OK I think, approach 1 should be our long term plan. This will require quite a bit of refactoring so we can take it as a tech debt or we can work on it incrementally as and when we touch these layers.
On Wed, Aug 15, 2018 at 5:17 PM Alex Amato <[email protected]> wrote: > 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, >> >>
