Merge lp:~elopio/u1-test-utils/log_in_from_redirect into lp:u1-test-utils

[Leo Arias]

Updated the SSO SST helpers for pay on staging.

[Vincent Ladeuil (vila)]

Where are the corresponding tests ?

review: Needs Information

[Leo Arias (elopio)]

<elopio> vila: about log_in_from_redirect, the corresponding tests are on pay.
<elopio> we decided against making unit tests for the actual page objects, as that would mean an additional place to change in case of a code change.
<vila> elopio: "an additional place to change in case of a code change" ? That's exactly why we want tests to cover code. The idea is that if you're modifying the code and introduce a regression, a test will tell you
<elopio> vila: a test will tell you, in pay.
<elopio> if you change the way pay goes from home page to log in, all the acceptance test will fail.
<vila> elopio: no, I disagree about splitting tests and code and people replied: don't worry, that's only for helpers
<vila> elopio: helpers need tests
<vila> elopio: otherwise we're adding even more test-debt
<vila> elopio: we just can't afford that
<elopio> vila: do you remember about the tests I added when we were on online-services-qa/sso-helpers ? They faked a log in page using the HTMLStringPage. We all reviewed that and came to the conclusion that it wasn't a good idea.
<elopio> I'm not sure how happy you were with that decission, I suppose not much :)
<vila> elopio: no I don't, sorry :-/
<vila> I mean: I don't remember, not I'm not happy ;)
<elopio> vila: let's say you need to change some detail on the devices page. Currently, that will mean a change in the real code, a change in the unit code, and a change in the acceptance tests helper.
<elopio> if it changes the user flow, also a change in the acceptance test.
<vila> elopio: yes
<elopio> that's potentially 4 changes.
<vila> elopio: hold on a sec
<vila> elopio: the flip side is:
<elopio> if we had a test for the acceptance test helper of the devices page, that would be an additional change with not a lot of value.
<vila> if you don't have to do this 4 changes, it means your code is not tested
<vila> untested code is broken code
<vila> or said more gently:
<elopio> as the acceptance test is already excercising that code path, will fail in case of an error, and we have alternate ways to check things like doing complementary tests, i.e., the things you add should have tests for removal too.
<vila> until you have a test showing that your code is working the way you think it is, you may be wrong and bug can (and will !) occur
<elopio> vila: I agree. The 4 tests for a change that affects the user are required. The fifth change (the test for the test helper) is the one that doesn't add value.
<vila> elopio: let's talk about removing tests when we have too much of them, for now, we haven't
<vila> elopio: as a reviewer of your mp, there is no way for me to run the tests that cover the changes you're proposing
<elopio> vila: we have lots of tests. On SSO, this code is used by 100 tests.
<vila> elopio: as a reviewer of your mp.. wait I already said that
<elopio> no, we are on pay. On pay, this code is used by 49 tests.
<elopio> vila: as a reviewer, you would start on the pay branch that requires this change.
<vila> elopio: then it should be easy to add a single one in u1-test-utils right ?
<elopio> if you want, you can run the tests. That's not easy though, and will take a lot of time.
<elopio> vila: it's easy t...

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Voting does not meet specified criteria. Required: Approve >= 1, Disapprove == 0, Needs Fixing == 0, Needs Information == 0, Resubmit == 0, Pending == 0. Got: 1 Approve, 1 Needs Information.

[Vincent Ladeuil (vila)]

review: Abstain

[Leo Arias (elopio)]

<elopio> vila, jfunk: http://paste.ubuntu.com/5619580/
<elopio> that's a test that fails without my change, and that doesn't use the browser so doesn't need a real or faked SSO.
<elopio> probably, it would look nicer starting a patcher for all the mocks. I was just trying to do it in 20 minutes :)
<elopio> does it suffice for you?
<vila> elopio: urhg, mocks all over the place :-/ And you'll tell me that it will be tested against real code somewhere else ?
<elopio> vila: on pay. With a real browser and a real server.
<vila> that's a clear demonstration that mocked tests allows one to write complicated tests (it's really hard to read and requires an almost complete knowledge of the implementation to understand)
<elopio> that's what I told you in the meeting.
<vila> so instead of helping sharing the knowledge, it *requires* the knowledge
<elopio> the only way to avoid the fakes was to mock everything.
<vila> I probably missed when you said 'mock' then
<vila> elopio: every time someone tells me "that's the only way", I know they're wrong. In software there is never a single way to do something
<elopio> this one is cleaner: http://paste.ubuntu.com/5619597/
<elopio> vila: if you don't mock those actions, you need a real or faked browser. Or mock the actions on sst.
<elopio> mock the actions on sst would require even a deeper knowledge of the implementation.
<vila> elopio: I think I was clear about wanting a fake...
<vila> the fake *describes* the knowledge
<vila> and the requirements for the test in this case
<vila> that's the exact opposite of what you're doing here
<elopio> just as the test_register_new_user in pay does, it describes the knowledge.
<elopio> that's why I don't want to maintain another test or a fake.
<vila> elopio: looks like you don't get what I call knowledge here :-(
<vila> elopio: either your helper can be defined without links to external apps and you should be able to test it, or it can't and it needs to designed differently
<vila> s/to designed/to be designed/
<vila> that's what I was advocating during the meeting
<-> nessita is now known as nessita-teaching
<elopio> vila: maybe you can start proposing alternative designs.
<elopio> I'm currently writing the test you want.
<elopio> that will take a bit longer.
<elopio> but the problem it's not writing it. The problem is writing it for all the helpers.
<vila> elopio: the first test is the hardest to write, focus on that one, we'll see for the others later (I didn't realize they were missing :-/)
<elopio> vila: it would be just as hard everytime you have to fake a page.
<elopio> They are not hard to write, it's easy, take a look at the test_page.py
<elopio> the problem is maintaining them. With little values in my opinion, as I consider them duplicated.
<vila> elopio: I'm afraid you won't be able to get to the point where each project tests what it should if you start with the idea that you will end up with duplication
<vila> elopio: instead, try to find how to reach the point where each project only has to check what it cares about
<vila> elopio: and what matters about test failures (excluding the flaky ones) is not that you have to fix them but that they are trivial to fix w...

[Leo Arias (elopio)]

Vila, Julien,

So, after a loooong discussion, we came up with the possible solutions.

I implemented two, so you can take a look and compare them on a better
way. Let me make a summary

1. Do nothing:

This is not the same as leaving untested code. This means that all the
code paths for the create_new_account helper in u1testutils are
exercised by the test_register_new_account in the pay project. If
something is wrong on the helper or if the code in pay changes, we will
notice immediately because we will be having a test failing. To fix
something in this helper means making this test pass.

Of course, I'm still in favor of this solution.

The problem is that the test is in a different project than the code.

2. Add a test without a server:

This was what you proposed at the end of the meeting. The only way (I
can find) of testing the fix without a fake or real server is mocking
the actions and just checking they were called.

http://paste.ubuntu.com/5619597/

The test that's there will fail until we merge the fix I prepared on the
original MP.
The problem is that it's mocking almost everything, thus the value of
the test is small.

3. Add a test with a fake server:

I think this is what Vincent likes. I tried to implement it with fake
html pages:

http://paste.ubuntu.com/5619829/

There are still some rough details as it's just a prototype, but the
idea is there.
It took me like one hour to make the test, so the reason I don't like
this solution is not because it's hard or I'm lazy.

I don't like it because if a dev wants to change the heding1 that says:
are you new?, now he has to change the original html, the page object
and the fake we have in this test.
It's even worse if he wants to change the flow or registration (as they
are doing with the new theme). Then a change would be required in the
html templates, in the tests for those templates, in the pay acceptance
test_register_new_user, in the u1testutils helpers, in the u1testutils
page object and in the faked page added by this test.
I would appreciated having one less change to do.

Also I don't like it because this is pretty simple to do; but we also
have helpers for the log in, for complete a payment and for changing the
payment preferences. Then we will end with many more than two faked
htmlstringpages.

4. Add a test with a real server:

The test would be copying test_register_new_user from pay to
u1testutils. It will look a lot like the test in (3), but instead of
opening an string page, we would open either a local SSO or staging SSO.
Currently starting a local SSO takes a lot of time, so I don't like that
way.
And I liked the idea of staging SSO, I started using that for the API
helpers, but failed repeatedly as staging SSO was changed or being
updated. So now I don't like it.

===============================

I think that the penalty for doing (1) it's not big, and I consider
(2-4) to be duplicating the same thing we are testing on
test_register_new_user, adding a new layer we need to maintain, and with
small added value.

That's my opinion, I rest my case. I'm showing here I can implement any
one of the solutions, and as what I want now is just to make pay tests
to go green a...

=== modified file 'u1testutils/sso/sst/__init__.py'
--- u1testutils/sso/sst/__init__.py	2013-04-16 06:31:16 +0000
+++ u1testutils/sso/sst/__init__.py	2013-04-24 04:24:27 +0000
@@ -32,15 +32,11 @@
         information will be send to it. Default is True.
 
     """
-    log_in = pages.LogIn()
+    log_in = pages.LogInFromRedirect()
     create_account = log_in.go_to_create_new_account()
     create_account.create_ubuntu_sso_account(user)
 
-    if is_site_recognized:
-        log_in.log_in_to_site_recognized(user)
-    else:
-        # User is already signed in so SSO will just show the site not
-        # recognized page.
+    if not is_site_recognized:
         site_not_recognized = pages.SiteNotRecognized()
         site_not_recognized.make_all_information_available_to_website()
         site_not_recognized.yes_sign_me_in()
@@ -85,7 +81,7 @@
     """
     if is_site_recognized:
         if user is not None:
-            log_in = pages.LogIn()
+            log_in = pages.LogInFromRedirect()
             log_in.log_in_to_site_recognized(user)
         else:
             # User is already signed in so SSO will just redirect back to the
@@ -93,7 +89,7 @@
             pass
     else:
         if user is not None:
-            log_in = pages.LogIn()
+            log_in = pages.LogInFromRedirect()
             site_not_recognized = log_in.log_in_to_site_not_recognized(user)
         else:
             # User is already signed in so SSO will just show the site not

=== modified file 'u1testutils/sso/sst/pages.py'
--- u1testutils/sso/sst/pages.py	2013-04-18 01:11:28 +0000
+++ u1testutils/sso/sst/pages.py	2013-04-24 04:24:27 +0000
@@ -46,7 +46,6 @@
 
         """
         self._log_in(user)
-        return YourAccount(user.full_name)
 
     @log_action(logging.info)
     def log_in_to_site_not_recognized(self, user=None):
@@ -88,9 +87,25 @@
     @log_action(logging.info)
     def go_to_create_new_account(self):
         """Go to the Create new account page."""
+        self._click_create_new_account()
+        return CreateAccount()
+
+    def _click_create_new_account(self):
         new_account_link = sst.actions.get_element(id='new-account-link')
         sst.actions.click_link(new_account_link)
-        return CreateAccount()
+
+
+class LogInFromRedirect(LogIn):
+
+    url_path = '/.*/\+decide'
+    is_url_path_regex = True
+    headings2 = ['Log in', 'Are you new?']
+
+    @log_action(logging.info)
+    def go_to_create_new_account(self):
+        """Go to the Create new account page."""
+        self._click_create_new_account()
+        return CreateAccountFromRedirect()
 
 
 class PageWithAnonymousSubheader(u1testutils.sst.Page):
@@ -149,6 +164,12 @@
         sst.actions.click_button(continue_button)
 
 
+class CreateAccountFromRedirect(CreateAccount):
+
+    url_path = '/.*/\+new_account'
+    is_url_path_regex = True
+
+
 class AccountCreationMailSent(PageWithAnonymousSubheader):
     """AccountCreation mail sent page of the Ubuntu Single Sign On website.