Merge lp://staging/~benji/landscape-client/better-self-signed-cert-ux-3 into lp://staging/~landscape/landscape-client/trunk

Looks good, and seems to work.

The merge description is wrong however - this branch "only" refactors the current code to make it easier to grok and test, but there is currently no functional improvement (that should come in a subsequent branch).

Note (can be used in the change description):
Most of the code breaks out nested function declaration to make them easier to test, using functools.partial to keep the invocation sane in a twisted context (test them "flat", use functools to pass closures to event handlers).

Note 2:
We used manual stubs instead of mocks for readability and personal preference (of both of us).

I'll comment again when I will have performed actual manual tests (give me a couple of hours :) )

> Looks good, and seems to work.
>
> The merge description is wrong however

I've fixed the MP description and incorporated your notes below.

> I'll comment again when I will have performed actual manual tests (give me a
> couple of hours :) )

/me crosses fingers.

Summary testing succeeded.

+1

Seems to work fine against my self-signed Landscape. A few comments inline.

I am not too happy with the tests, since you seem to be mostly testing the wiring, and never that it actually works well together. I do understand that coming up with proper tests would require even more work, and with the understanding that this does not make the code base any worse, I am approving it.

Finally, I am not sure it's such a great idea to have Chris do one of the reviews _if_ he has contributed a lot of code/opinions while you were developing this branch. But you guys know best how much his mind is already "tainted" with the approach :)

I will abstain from this one since I peer-programmed some of it.

Good point Danilo, I added another review slot and abstained.

Why couldn't this be broken up into smaller branches?

> Why couldn't this be broken up into smaller branches?

Agreed. I didn't look at the code in detail, but I was expecting to see at least 2 branches: one for the pure refactoring of the registration code and one for adding the new functionality.

Free, this *is* the pure refactoring branch.

> Free, this *is* the pure refactoring branch.

In that case, the MP description is wrong. It says that more info is provided to the user and more error conditions are handled.

> > Free, this *is* the pure refactoring branch.
>
> In that case, the MP description is wrong. It says that more info is provided
> to the user and more error conditions are handled.

I had some sort of edit failure and didn't get the MP description corrected. It is right now.

> Why couldn't this be broken up into smaller branches?

It surely could have been, but given the nigh untestable nature of the current code structure (even after our refactoring), one large branch was the least risky way to proceed without undue exploratory testing, especially given Chris' upcoming disconnectedness.

> Seems to work fine against my self-signed Landscape. A few comments inline.
>
> I am not too happy with the tests

As are we. Unfortunately, better tests would have required much more refactoring as well as having some place to put integration tests, which were expressly forbid when the idea of adding some to the existing test base was floated.

> Finally, I am not sure it's such a great idea to have Chris do one of the
> reviews _if_ he has contributed a lot of code/opinions while you were
> developing this branch.

Christ has recused himself from the review.

(I am of the opinion that his review would have been fine, given that I am not big on two-person reviews in the first place, and more so if a branch is the result of pair programming -- which this was.)

Yes, agreed re better tests: I don't think it's worth it.

I am happy about the Christ not having an opinion on this branch :)

As for Chris, pair programming usually does lead to better code, but at the same time can result in two minds agreeing to decisions and never re-questioning them after the original agreement (I've been guilty of the same thing).

Anyway, my overall assesement is that this is improvement over the existing code (and tests are no worse) which is reflected in my approval :)

Thanks for the good review Danilo. I fixed the things you pointed out inline. I also noticed some of the pre-existing tests were generating output to stdout, so I corrected those.

Sorry, I won't have time to finish this review before my EOD. I added some initial comments inline.

Ok, I have had time to look at this branch more closely now. How would you feel if I put it in Needs Fixing and requested you to rewrite the tests?

I understand that it's some work doing that, and that fakes are a small improvement over mocking, but the emphasis is on small here. The way you use fakes makes it very similar to mocking. It's still easier to read, but you're still testing the that the code is written in the same way the tests expects them to be.

I would rather that the tests were a bit more functional, in that you set up the conditions, then call register() and check what it returns. I don't care how the deferreds are set up internally. What I care about is that register() returns "success" when the registration succeeds.

It's not quite trivial, but here's an example of a test that checks that register() returns "success" when the registration is successful:

  https://pastebin.canonical.com/126660/

What do you think?

> What do you think?

I haven't taken the time to look deeply into your proposed test approach, but I agree with everything you said. Unfortunately, the amount of time invested in this branch thus far means that the size of the marginal improvement must be very large for it to be worth it and regrettably, that's not the case.

I plan on keeping the test approach in mind for the next time we work on this code (which I hope is soon, it needs quite a bit of help).

Thank you for the review.

Ok, I won't push too hard on adding better tests, since the existing tests weren't great either. But the old tests did actually tests that the async machinery really works, which your tests don't. So I would like you to at least add the test that I pasted, so you have one test that does end-to-end testing.

I'd also like you to respond to the inline comments I added here, as well as those I added previously, and push the latest revision please. I still don't see the changes you did in response to Danilo's comments.

> [...] So I would like you to at
> least add the test that I pasted, so you have one test that does end-to-end
> testing.

Done. (BTW, that diff was odd. I'm curious how you generated it. I had to hand apply it.)

> I'd also like you to respond to the inline comments I added here, as well as
> those I added previously

Done.

> and push the latest revision please.

Done.

Sorry for being a pain :) But the latest diff isn't what I would expect it to be. It lacks at least one change that you said you would do (I added an inline comment), and the test I added isn't there. Instead you added a bunch of other tests that do mocking and seem redundant?

BTW, maybe the patch was odd because you looked at the wrong pastebin? ;) Although, the patch could look odd since I based it on what you pushed. Actually, looking at what you pushed, it's quite clear that you didn't push the changes you did in reply to my comments. You pushed revisions from Feb 26.

Please make it a habit to always push the latest code to LP. It's a bit complicated to review the branch when you know that there are other changes that you can't see.

> Sorry for being a pain :) But the latest diff isn't what I would expect it to
> be.

I pushed, but apparently forgot to commit beforehand.

> BTW, maybe the patch was odd because you looked at the wrong pastebin? ;)

This is the pastebin I was referring to: https://pastebin.canonical.com/126660/

The oddities I see are the blank lines with a dot in the indicator position and the lack of whitespace after the file name in the headers.

Ok, let's get this branch landed now!

> This is the pastebin I was referring to: https://pastebin.canonical.com/126660/
>
> The oddities I see are the blank lines with a dot in the indicator position and the lack of
> whitespace after the file name in the headers.

Oh, that is odd indeed. I've never seen that before, but it seems to be since I copied
it from 'bzr diff | vim -' and the diff highlighter adds those periods.