On 4 June 2014 02:45, Kohsuke Kawaguchi <[email protected]> wrote:
> > Wait, what? > > When you break a test unintentionally and someone else kindly points that > out to you, you'd say "oh, thank you very much", then you go on to try to > fix a problem. > Instead you decide to attack the test, claiming that it's brittle, and > demand that someone else fixes it. > Nope. I said that the test harness does not let the tests be written the correct way and that I was going to fix the test harness first *and* then fix the tests after I had fixed the acceptance test harness > > That is not nice. > > Here are the tests: * login_ok * login_wrong_password * login_no_such_user * login_no_ldap * login_search_base_people_ok * login_search_base_people_not_found * login_email_ok * login_use_fallback_server * resolve_email * resolve_group_membership_with_defaults * resolve_group_membership_with_defaults_negative * custom_group_search_base * resolve_display_name_with_defaults * custom_display_name * custom_group_membership_filter * custom_mail_filter Which tests should correctly be broken by the change I made? I argue that if the tests were written correctly there should be none of those tests failing as a result of my changes. They should be driving via the config file and the config file should be backwards compatible per our contract of Jenkins. I could accept the argument that one test should fail, i.e. * custom_group_membership_filter However my view is that there is a missing test (or more correctly two missing tests if testing the new plugin version) and that those missing tests are the tests that should have failed and custom_group_membership_filter should not have failed. So in the case of custom_group_membership_filter, I agree that there needs to be a test that verifies that you can change the group membership filter via the UI... but I disagree on how that test should be written. What such a UI test should be doing is verifying that the UI changes get propagated to the configuration file. Then then the other tests which use config files can be relied upon to provide coverage. That way you get test failures that clearly point to the source of the problem an allow one to clearly resolve them. The good news is that these tests are close to the right path. Fixing them to the right path is not too much work, e.g. all the login_* and resolve_* tests just need to have the test setup not drive via the UI. Their verification of the configuration via the UI can be left intact. In fact as the tests are currently structured we can just rewrite two methods and make a few minor fixups and the tests will then be on the right path > > It looks like the change we are talking about is [1]. This involves > pulling out a hard-coded functionality (LDAP group filter) and demoting it > to an implementation of an extension point. > > This is a big change that spans both UI and functionality. Of course it > breaks a test, and that's a test's way of asking you if you really meant to > change that part of the behaviour. > Look at the scenarios described in all those tests. None of them start with "When I configure Jenkins to have LDAP security realm configured with blah blah blah". Instead they all start from "Given LDAP is already configured in way XYZ." Thus the test part of those tests is the When ... Then ... None of these scenarios are supposed to be testing the UI for configuring LDAP from the test comments. So if we are following the test comments then in fact my change should not have broken any of these tests if they were written correctly. Note I am not criticising Michael, who wrote the best tests he could given the inadequacy of the test framework. The issue here is that the current acceptance test harness does not let Michael write the tests the right way. Should there be a test case that covers the ability to configure via the UI? Yes there should... but it can safely stop once the config file has been persisted to disk. > So you change the test, to re-confirm that you intended it. Just like you > had to update src/test/java/.../LDAPSecurityRealm_Test.java in this > change. This is a test working as it is supposed to be. > > > These tests cover a lot of things, including UI. I don't understand the > notion that this is somehow bad and that we actively should avoid testing > the UI, when it is one of the problems we often fail to catch in unit tests. > Well the issue is retaining confidence in tests when you change things. Every automated test runs the risk of a false positive, i.e. where the test passes despite the feature being broken. When you are writing the test and driving everything via the UI, you are watching the browser window go through the setup and you can see if your test has wandered off the path... say a form autocomplete fixed something when you had selenium tab to the next field... and hopefully you structure your test to avoid such pitfalls. So the test you wrote does not currently have a false positive. Enter a simple change in the system. Your test stays passing... has a false positive crept in? Well we didn't change any of the UI driving code and we didn't change the test case itself so perhaps that test is still good Now enter a UI change. Because of the UI change we now have to change the page objects that we are using to drive the test. As soon as we change the page objects we have now propagated a risk of false positive to *every* test case that uses those page objects. So in the case of the LDAP tests, one change means we have to revalidate 17 test cases against false positives. We should only have to revalidate those test cases that are specifically testing configuration via the UI. All the other test cases should not need revalidation because their When-Then parts do not call out for configuring the LDAP. All the stuff in Given should be driven outside of the UI > > They are used to qualify LTS releases, and they are currently the only > tests that actually run LDAP plugin with a real LDAP server. I'm sure there > are ways to make it better, but these tests have unquestionably positive > values as they stand today already. > > But you and I disagree, and I respect your choice of not writing these > tests for your code. Please understand that what I have said is that I am not fixing those tests *now* _because_ I intend to fix the harness first before fixing the tests. > Likewise, please show some respect to what other people are doing in the > community. > You know my view is that we are rushing headlong into writing loads of tests that are ever so slightly on the wrong path. Every time we change a test or the code backing a test, we run the risk of introducing a false positive. Until we are in a place where the Given part of a scenario can be set up outside of the UI[2] all we are doing is creating more tests that we will have to re-verify against false positives once I integrate the support for that. I respect the community wanting to write tests... I just want to ensure that we are writing the right kind of tests. GIVEN: the number one thing that people want to fix in Jenkins is the UI WHEN: people expect that tests give confidence that upgrading their existing Jenkins is OK THEN: the majority of tests should drive via the config files rather than the UI [2] Now if the last part of a Given is something like "I am on the XYZ screen with ABC selected" then that is fine to setup via the UI... but basically we should try to minimise what we drive via the UI to the bare minimum > > > [1] https://github.com/jenkinsci/ldap-plugin/commit/ > a375a65b639447d22807f3d03d9d5a3e73637fc1 > > > On 05/24/2014 06:49 AM, Stephen Connolly wrote: > >> I am not planning to update UI driven tests of non UI fubctionality as I >> fundamentally disagree with that approach. I will probably replace these >> tests >> with a non-UI driven version when I have integrated my scalability test >> framework into the acceptance test harness which will enable the writing >> of >> sensible tests and leave UI driven tests for what they are best for... Ie >> testing the ui >> >> On Saturday, 24 May 2014, Ullrich Hafner <[email protected] >> <mailto:[email protected]>> wrote: >> >> Are you planning to update the test? >> >> Am Freitag, 23. Mai 2014 schrieb Stephen Connolly : >> >> Well if the tests are non UI tests driven through the UI then >> they can >> be overly brittle. >> >> @Kohsuke this is a case in point >> >> On Thursday, 22 May 2014, Ulli Hafner <[email protected]> >> wrote: >> >> Seems that the new plugin breaks the acceptance tests for >> the LDAP >> plugin: >> https://github.com/jenkinsci/acceptance-test-harness/blob/ >> master/src/test/java/plugins/LdapPluginTest.java >> >> Am 22.05.2014 um 17:12 schrieb Stephen Connolly >> <[email protected]>: >> >> OK, so there is now rumoured to be a faster and better way >>> to look >>> up the groups that a user belongs to in the LDAP 1.10 plugin. >>> >>> I say rumoured because due to the complexities of Active >>> Directory >>> server configurations, one can never be quite sure until one >>> has >>> had a fair amount of testing. >>> >>> To that end, please could you set up a simple test Jenkins >>> instance and upgrade to ldap:1.10 and configure the `Parse >>> user >>> attribute for list of groups` group membership strategy >>> (again >>> rumour has it that on Active Directory the attribute >>> `memberOf` is >>> the magic attribute. >>> >>> See if that ends up giving you the same JENKINS_URL/whoAmI >>> list of >>> groups as when you have the `Search for groups containing >>> user` >>> set with the filter being >>> `(member:1.2.840.113556.1.4.1941:={0})`... though the >>> `Parse user >>> attribute for list of groups` should be very very fast for >>> login >>> while the `Search for groups containing user` could take >>> *ages*. >>> >>> Respond back here with your findings so that I can remove >>> the Red >>> text on the version history about this being a rumour >>> >>> -- >>> You received this message because you are subscribed to the >>> Google >>> Groups "Jenkins Users" group. >>> To unsubscribe from this group and stop receiving emails >>> from it, >>> send an email to jenkinsci-users+unsubscribe@ >>> googlegroups.com. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> >> >> -- >> Sent from my phone >> >> -- >> You received this message because you are subscribed to the >> Google >> Groups "Jenkins Users" group. >> To unsubscribe from this group and stop receiving emails from >> it, send >> an email to [email protected]. >> For more options, visit https://groups.google.com/d/optout. >> >> -- >> You received this message because you are subscribed to the Google >> Groups >> "Jenkins Users" group. >> To unsubscribe from this group and stop receiving emails from it, >> send an >> email to [email protected] >> <javascript:_e(%7B%7D,'cvml','jenkinsci-users%2Bunsubscribe@ >> googlegroups.com');>. >> >> For more options, visit https://groups.google.com/d/optout. >> >> >> >> -- >> Sent from my phone >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Jenkins Users" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email >> to [email protected] >> <mailto:[email protected]>. >> >> For more options, visit https://groups.google.com/d/optout. >> >> > > -- > Kohsuke Kawaguchi | CloudBees, Inc. | http://cloudbees.com/ > Try Jenkins Enterprise, our professional version of Jenkins > > -- > You received this message because you are subscribed to the Google Groups > "Jenkins Developers" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Jenkins Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
