On Fri, Feb 07, 2014 at 01:54:46PM -0500, Chris Evich wrote: > On 02/07/2014 11:59 AM, Ademar Reis wrote: > >> 4b) Unittests for core code must not depend on system (i.e. external > >> tools, configuration, or data) -or- repository state (i.e. higher-level > >> module availability or present TP code/tests). These are key to early > >> organizational problem detection. > > > > Agree that core could should never *ever* depend on TP code. > > We should probably be even more super-duper clear here: > > The core should never *ever* depend on the implementation details or > behavior of TP. However, it may (and often must) depend on a TP's > following a core-defined interface very closely.
The core should provide an API just like any library and TPs should work on the expectation that the API works and is stable. Core code should have zero knowledge or expectation of what happens inside the TPs. BTW, I bet we can extend check_patch.py to check if code added to virt-test has any references to code found in the test providers. Ditto for code added to TPs that make reference to code from other TPs. > > >> > >> 4c) TP unittests on the other hand, _SHOULD_ make liberal use of > >> available code-code and unittests. However, dependence on system state > >> is still forbidden. These are also key to early organizational problem > >> detection. > > > > As long as there are no TP interdependencies, yes. > > I agree in principle, though these few words appear to hide a massive > spider-web of work in practice :S I think it's the price we paid for > having a large growing community, and promoting 'organic', rapid > development. I think the depth and scale weren't fully realized until > recently. Agree and I think it was a good and maybe even necessary trade-off. But it's never too late to start fixing it. > > > So depending on external configuration/tools/state is acceptable, > > as long as the test is disabled by default and properly > > documented. Most of the time, this is better than no test at all. > > Based on several actual experiences on the wrong-side of this, not > having the unittest is more practical than disabling it by default. > This is because when a unittest can't be segregated properly, it's often > sign of massive underlying code interdependence / layering problems. In > the spirit of 'get 'er done', re-writing the underlying code is often > impractical, and then it's better to just skip that unittest and rely on > CI catching problems. Otherwise you risk having to fix both the code > and the unittests later. Maybe we're talking about different things here. What I mean by depending on external tools is, for example, the dependency for a specific hardware, host configuration or something non-free being installed. In such cases, I advocate that it's OK to add a test that is disabled by default alongside with a README explaining what needs to be done to run it. Another approach is the test to automatically detect such dependency. For example, the database tests may run only if MySQL is enabled, otherwise they're skipped. BTW, the recent work by Cleber on the database tests is a good example of when maintaining mocks for the tests is way too expensive to be worth it. If the real code officially supports MySQL (only) but the mock runs on top of sqlite, what we're actually doing is: 1. Write extra backend code to support both MySQL and sqlite 2. Test only the sqlite backend In the end, this may backfire, because the extra code for (1) may introduce problems in the officially supported backend that (2) will never catch. In such a case having no test (or tests that require a specific environment but are disabled by default) is a better alternative. > > > > Although in practice I think what you've described will happen > > naturally, I suggest we leave it out of the "rules" or "policy" > > I'd like to agree, and I hope you're right. I wrote those based on > LMR's concern of "we had rules in the past, but nobody followed them". > In the past, these kinds of fixup responsibilities seem to land on LMR's > plate (and/or he's just a _really_ nice guy). > > I'm not sure if he or other maintainers have time dedicated for > this or not. Rather it just seemed a valid concern worth > mentioning. And maybe you're right. If this is already perceived as a problem, then yes, it should be stated in the policy. Cheers. - Ademar -- Ademar de Souza Reis Jr. Red Hat ^[:wq! _______________________________________________ Virt-test-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/virt-test-devel
