launchpad-reviewers team mailing list archive

["j.c.sackett" <jcsackett@xxxxxxxxxxxxx>]

> - Please make sure to link the test files to the build directory vs the local
> one. Basically we're ensuring that you're testing what's in the build which
> the combo loader will serve. In this way part of the test is verify that the
> files are getting 'built' correctly. (mustache, banner)

Hm. I think I can see your point, but the idea feels...off? One of the nice bits
of YUI tests is that you can just "code and reload"; running off things in `build`
breaks that. Is this a suggestion, or is it our policy? If not the latter, I really
think it may be worth a -dev discussion to sort it, b/c there's a lot of tension
between those two ideas from where I stand (or sit :-P).

>   Also looks like you moved the test_information_type_choice.html links?

I did, for the reasons I mentioned--having them on `build` seemed wrong (and not all
of our tests do that, last I checked). Again, I can move these back, but it seems
wrong from my POV. If we have concern about files being built correctly, it feels like
we should have separate test infrastructure that verifies that.

> - Another case of please move the #maincontent node creation to the setup() so
> that teardown isn't actually setting up the next test.

Yeah, there's a lot of remaining cleanup in these branches. I'll do some before landing,
and I'm also planning on doing a final branch when I'm done with the actual bugs to
cleanup remaining goofiness.

> - The test in test_information_type_choice.js I'm a bit leery of firing global
> show/hide events. I can't think of anything that'd be listening, but there
> seems to be a big potential of triggering something listening out there
> unknowingly. What about just namespacing those a bit more to be testBannerShow
> or something?

Sure, I can make that change.

> - In that same test file you removed the dep on lp.app.privacy, but didn't add
> in the new banner? It looks like it's still needed in the shim code.

There's a requirement for lp.app.banner.privacy, which is in the requires; both banner and banners/privacy are included in the html reqs.

> - In line 678 of the diff you don't use the getPrivacyBanner? Is there any
> reason for the change in usage there?

Yes--in that instance the banner is being created with different text. Because it's on +filebug, which will not have a `private` class on the `body` element, we know that there won't already be a banner within that page load, and there will be a reload before the private bug gets loaded, so we don't have to worry about this instance colliding with other calls.
-- 
https://code.launchpad.net/~jcsackett/launchpad/use-banner-to-cleanup-privacy/+merge/105731
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.