+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 thrown for null options" or "ordering of parameters" not being strictly necessary and definitely not something that should be used as reference for other IOs, (staying consistent with other IOs is probably the reason why DatastoreIO has some of them), I still think some degree of leeway for the author is desirable depending on what level of user experience they want to provide, as long as it is not a burden for adding new tests. On 11 March 2017 at 14:16, Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > +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 unit-testing the trivial builder >> methods. >> >> E.g. a significant part of >> https://github.com/apache/beam/blob/master/sdks/java/io/goog >> le-cloud-platform/src/test/java/org/apache/beam/sdk/io/ >> gcp/datastore/DatastoreV1Test.java >> is >> devoted to that, and a significant part of the in-progress Hadoop >> InputFormat IO. >> >> In particular, I mean unit-testing trivial builder functionality such as: >> - That setting a parameter actually sets it >> - That setting parameters in one order gives the same effect as a >> different >> order >> - That a null value of a parameter is rejected with an appropriate error >> message >> - That a non-null value of a parameter is accepted >> >> I'd like to come to a consensus as to how much unit-testing of such stuff >> is appropriate when developing a new transform. And then put this >> consensus >> into the PTransform Style Guide >> <https://beam.apache.org/contribute/ptransform-style-guide/>. >> >> I think such tests are not worth their weight in lines of code, for a few >> reasons: >> - Whether or not a parameter actually takes effect, should be tested using >> a semantic test (RunnableOnService). Testing whether a getter returns the >> same thing a setter set is neither necessary (already covered by a >> semantic >> test) nor sufficient (it's possible that the expansion of the transform >> forgets to even call the getter). >> - Testing "a non-null value (or an otherwise valid value) is accepted" is >> redundant with every semantic test that supplies the value. >> - For testing of supplying a different order of parameters: my main >> objections are 1) I'm not sure what kind of reasonably-likely coding error >> this guards against, especially given most multi-parameter transforms use >> AutoValue anyway, and given that "does a setter take effect" is already >> tested via bullets above 2) given such a coding error exists, there's no >> way to tell which orders or how many you should test; unit-testing two or >> three orders probably gives next to no protection, and unit-testing more >> than that is impractical. >> - Testing "a null value is rejected" effectively unit-tests a single line >> that says checkNotNull(...). I don't think it's worth 10 or so lines of >> code to test something this trivial: even if an author forgot to add this >> line, and then a used supplied null - the user will just get a less >> informative error message, possibly report it as a bug, and the error will >> easily get fixed. I.e. it's a very low-risk thing to get wrong. >> >> I think the following similar types of tests are still worth doing though: >> - Non-trivial parameter validation logic: where the validation rule is >> complex, or where the error message is interesting, etc. >> - Cross-parameter invariants which, if violated, lead to data loss: e.g. >> that if parameters foo and bar are mutually exclusive, verify that if the >> user specifies both, they get an error, instead of one of them being >> silently ignored. >> >> Thoughts? >> >> > -- > Jean-Baptiste Onofré > jbono...@apache.org > http://blog.nanthrax.net > Talend - http://www.talend.com >
