You seem to believe that the only purpose of a test is to test the specific bug or change for which it was introduced. That is indeed the philosophy of test-driven development, but there are more things in heaven and earth than TDD.
Calcite is a complex project, and needs complex tests to shake out complex issues. We do not have a QA team to write tests. Therefore the tests will be written during the course of fixing bugs. Julian > On Sep 9, 2018, at 1:48 PM, Vladimir Sitnikov <[email protected]> > wrote: > > Julian>I strongly disagree with your commit 88f125541. You have removed test > code that is useful in creating a high-quality product. > > Could you please provide technical justification? > > Note: the test is still there. > Note2: the test still runs in both Travis and Apache Jenkins. The test > still runs from mvn test by default. > > I'm inclined to suppose your "removed test code" wording is not quite right. > > Note2: it still verifies that ARBITRARY order results in more rule > applications than DEPTH_FIRST. > Note3: original test code did not verify that the result of Hep planning > matched to the expected one. > > Juilan> I think you should back out your commit. > > I think I should not. > > Julina>I sent a previous email on the subject but you did not respond. > > I'm afraid I fail to see which response do you expect. > My opinion on the testRuleApplyCount test is as follows: > a) It is useful to compare ARBITRARY and DEPTH_FIRST implementations. > DEPTH_FIRST wins there, and it does NOT require to do a 1000'000 iterations > to prove that. > That is why I reduced test complexity so a unit does not exceed 5 seconds. > > b) The SQL+Hep program result in bad scalability for Calcite. In other > words, as the number of unions grows, planning becomes very slow. While it > is interesting, and it might be even a bug in Calcite, I do not see how > executing such a test (on every Travis build, on every Jenkins build, and > on every dev build) helps Calcite development. > > I can add a benchmark that would run that test with various numbers of > unions and print the response time. > However, we should use Apache and Travis' resources wisely and we should > refrain from burning CPU for little reason. > > I see your passion to build stable and robust software (that is great!), > however I'm quite sure HepPlannerTest#testRuleApplyCount is not something > that provides extra safety. > > Vladimir
