Code review comment for lp:~thomir-deactivatedaccount/canonical-identity-provider/trunk-add-gpg-key-management

On Wed, Apr 20, 2016 at 11:39 PM Natalia Bidart <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Hello Michael, Thommy,
>

Hi Natalia,

>
> I was a little bit surprised with the adding of testtools' TestCase and
> testtools.matchers to these tests. I understand the difference in using
> matchers vs standard TestCase assertion methods, and leaving aside the
> discussion whether is something we want or not, I would like us to talk
> about adding them to SSO or any other existing big project.
>

I had assumed that using testtools here would be a non-issue given that it
was already a dependency of the project - that's my fault if anyone's. And
yes, we're happy to bring that up on the list, and not use testtools here
if that's better for the team - but note, this wasn't about using
testtools.matchers but rather avoiding adding more tech-debt in SSO when
testing the SSO<->GPGService interactions.

>
> SSO is a large project with a lot of code, and in many past conversations
> the OLS team have agreed that given the amount of people working in every
> project, and given the importance of being able to switch between projects
> easily and fast, one of the things we value the most is consistency (on top
> of things that may be a nice to have in other contexts).
>

+100 for consistency - I'd not seen this as breaking consistency at all.
Perhaps a silly example, but if you grep the current SSO codebase for
"assertEqual.*True" and "assertTrue" you'll see that there are many tests
using:

assertEqual(result, True)

and many others using:

assertTrue(result)

 - which is fine, I've never felt the need to make those consistent because
they read well and have clear intent, as does assertThat(result, Is(True))
. There are obviously lots of other benefits to testtools matchers, but I
don't want to derail the main point here, rather just highlight why I
didn't see this as a consistency issue at all.

It was not because of matchers that we started using the existing testtools
dependency here...

>
> Starting using testtools for our unit tests is something that breaks
> consistency, and wasn't agreed at team level in advanced.

As above, I'd not even thought of the use of matchers as a consistency
issue but am happy to accept that it is for some people, so that's
completely my fault for not recognising it as such and bringing that up
with the wider team. We'll do that, and remove the use of matchers if
that's the consensus - that's not an issue nor the reason we used testtools
here...

> The testtools dependency was there only for the acceptance tests.
>
> Even more, using multiple inheritance on different base TestCases is
> something that has been proven to be a complete disaster in the mid term,
> we suffered from this in the Ubuntu One code where we had the Twisted
> TestCase, the Django TestCase, and the testtools TestCase as multiple base
> test cases. All the benefits a particular TestCase can give, are lost and
> become a burden when combining more than one as base classes.
>

The only issue with test inheritance that I'm aware of and totally agree
with the issue, is when individual tests are re-used via inheritance of
test-cases or gain added test state via inheritance - that can be very
confusing to work with and we've worked to remove those in the past (which
is why lifeless wrote https://pypi.python.org/pypi/testscenarios - which
depends on testtools - which allows you to do exactly that - re-use
unit-tests in different scenarios in an explicit and easy-to-reason-about
way). So I'm interested to hear what different issue you had (either here
or the mailing list might be better).

>
> For our Django projects, the OLS decision was to use the Django TestCase
> with a few extra, custom helpers defined.

I am completely unaware of the above decision and didn't even think of this
as an inconsistency issue - my fault. Happy to bring it up and revert if
it's best for the team.

> After a quick look, most matchers usage can be replaced with perfectly
> valid assertion from the Django TestCase:
>
> self.assertThat(keys, HasLength(1))
> self.assertThat(resp.content.decode('utf8'), Not(Contains('GPG Keys')))
> self.assertThat(resp.status_code, Equals(200))
>
> self.assertEqual(len(keys), 1)
> self.assertEqual(resp.status_code, 200)
> self.assertNotContains(resp, 'GPG Keys')
>

It wasn't because of matchers that we've used testtools here - assertions
can be expressed in quite a few different ways already in the codebase
(without inconsistency IMO) - but I don't particularly mind if the team
decides against matchers. We used the matchers because we were already
needing to use testtools to properly test the SSO UI interaction with the
gpgservice, without adding more Mocks or requiring a test double. Did you
get a chance to look at that part of the MP? I actually had hoped Thomi's
work here would be a great example of how we can test (smaller) external
dependencies within our projects without Mocks or otherwise adding more
tech debt.

We'll bring that up on the ML, and hopefully can see whether it's best as a
team to not use testtools at all, or just use certain features (like the
test fixture support for above SSO UI testing, but not the matchers) etc.
If it's best for the team to not use testtools at all, we may be able to
backport just the test fixture functionality manually and still avoid
mocking anything, but that will also be adding tech-debt of a different
sort (as opposed to just using test-tools).

>
> (note that the usage of assertContains/assertNotContains will also be
> clever about decoding the reponse, grabbing the text content, etc,
> something that otherwise is being made by hand and is a more error prone).
>
> For other more complex matchers, like these:
>
> self.assertThat(email, EmailContainsPGPBlock())
> self.assertThat(keys[0]['fingerprint'],
> FingerprintEquals(encryption_fingerprint))
>
> the existing methodology is to create custom assert helpers that isolate
> the specific logic needed for each case:
>
> def assert_gpg_block_in_email(self, email)
> def assert_fingerprint_encryption_in keys(self, keys)
>
> In summary, I would kindly ask you to maintain consistency in line within
> the existing project, or at least bring this issue to the whole OLS team
> and reach consensus

+100 for consistency as above - didn't realise this would even be an issue.
We'll send an email to the list and switch to non-testtools assertions if
that's best for the team, and not use testtools fixture support if that's
also best for the team.

> and have a migration plan before landing this to trunk.
>

I'm not sure why a migration plan would even be necessary... do you think
we'd need to switch to use matchers in all assertions?

Cheers,
Michael