Re: Style: how much testing for transform builder classes?

The new guidance has been submitted https://beam.apache.org/contribute/ptransform-style-guide/#testing-transform-construction-and-validation On Fri, Aug 18, 2017 at 10:48 PM Jean-Baptiste Onofré wrote: > Hi Eugene > > It sounds good to me. > > Regards > JB > > On Aug 18,

Re: Style: how much testing for transform builder classes?

Hi Eugene It sounds good to me. Regards JB On Aug 18, 2017, 21:42, at 21:42, Eugene Kirpichov wrote: >Hi all, > >This seems to have slipped through the cracks. I'd like to raise this >again >since I'm doing a cleanup in https://github.com/apache/beam/pull/3730 ,

Re: Style: how much testing for transform builder classes?

Hi all, This seems to have slipped through the cracks. I'd like to raise this again since I'm doing a cleanup in https://github.com/apache/beam/pull/3730 , and I'd like to get consensus and add the guidance to PTransform Style Guide or the Testing Guide. Let me rephrase my suggestions from the

Re: Style: how much testing for transform builder classes?

>From Ismael's and Dan's comments, I think I agree that there's a class of easy-to-make errors that justifies having some builder tests. 1. Errors of the form "withFoo() instead sets bar, which is of the same type as foo so it still compiles" (what Ismael quoted). This, I think, should be caught

Re: Style: how much testing for transform builder classes?

On Tue, Mar 21, 2017 at 5:14 PM, Dan Halperin wrote: > https://github.com/apache/beam/commit/b202548323b4d59b11bbdf06c99d0f > 99e6a947ef > is one example where tests of feature Bar exist but did not discover bugs > that could be introduced by builders. > True,

Re: Style: how much testing for transform builder classes?

https://github.com/apache/beam/commit/b202548323b4d59b11bbdf06c99d0f99e6a947ef is one example where tests of feature Bar exist but did not discover bugs that could be introduced by builders. AutoValue like alleviates many, but not all, of these concerns - as Ismael points out. On Tue, Mar 21,

Re: Style: how much testing for transform builder classes?

+1 to Vikas point maybe the right place to enforce things correct build tests is in the validate and like this reduce the test boilerplate and only test the validate, but I wonder if this totally covers both cases (the buildsCorrectly and buildsCorrectlyInDifferentOrder ones). I answer Eugene’s

Re: Style: how much testing for transform builder classes?

Yes, what I meant is: Necessary tests are ones that blocks users if not present. Trivial or non-trivial shouldn't be the issue in such cases. Some of the boilerplate code and tests is because IO PTransforms are returned to the user before they are fully constructed and actual validation happens

Re: Style: how much testing for transform builder classes?

Thanks all. Looks like people are on board with the general direction though it remains to refine it to concrete guidelines to go into style guide. Ismaël, can you give more details about the situation you described in the first paragraph? Is it perhaps that really a RunnableOnService test was

Re: Style: how much testing for transform builder classes?

​+0.5 I used to think that some of those tests were not worth, for example testBuildRead and testBuildReadAlt. However the reality is that these tests allowed me to find bugs both during the development of HBaseIO and just yesterday when I tried to test the write support for the emulator with

Re: Style: how much testing for transform builder classes?

+0.5 My two cents, * However trivial the test is it should be added unless user has a easy workaround to not having to wait for a few days until the trivial fixes are merged to beam and then propagated to the runner. * While I agree with trivial tests like "ensuring meaningful error message

Re: Style: how much testing for transform builder classes?

+1 Testing is always hard, especially to have concrete tests. Reducing the "noise" is a good idea. Regards JB On 03/10/2017 04:09 PM, Eugene Kirpichov wrote: Hello, I've seen a pattern in a couple of different transforms (IOs) where we, I think, spend an excessive amount of code

Re: Style: how much testing for transform builder classes?

+1 to reducing "trivial" tests such as these. More below. On Fri, Mar 10, 2017 at 7:53 PM, Kenneth Knowles wrote: > +0.5 > > Tests of trivial validation failures, if they check the error message, are > actually tests of effective communication in the error message

Re: Style: how much testing for transform builder classes?

+0.5 Tests of trivial validation failures, if they check the error message, are actually tests of effective communication in the error message (example: testReadValidationFailsQueryLimitZero). These are on easily on par with functionality tests in terms of importance for users. But tests of