launchpad development
ReviewerMeeting20090617
summary
we prefer (expected, observed) as the order of parameters to unittest assert* methods. preferred rather than enforce. (henninge to capture this in wiki)
- intellectronica on more maintainable javascript code:
- code that is OO and modularized is much easier to work with than code that is very imperative and/or relies on closure too heavily
- unit tests (aka YUI tests) are a great way to test our code. in many cases much better than integration tests
- working on UI features it is often easier to first concentrate on completing a working UI than on code quality. that's fine, but it means that we should refactor the code immediately after it passes UI review. that can happen either before code review, or if not, the reviewer should encourage doing that (either as part of this branch or in a followup branch).
- finally, since javascript code is so much more verbose, we propose a much higher limit, in order to encourage developers to not skip these important steps. 1600 lines
look in .../javascript/*/tests for unittest examples; only soyuz is in our tree atm. or look in lazr-js.
- intellectronica to email about higher limit.
logs
AMEU
Jun 17 10:00:26 <barry> #startmeeting
Jun 17 10:00:29 <MootBot> Meeting started at 09:00. The chair is barry.
Jun 17 10:00:29 <MootBot> Commands Available: [TOPIC], [IDEA], [ACTION], [AGREED], [LINK], [VOTE]
Jun 17 10:00:40 <barry> hello everyone and welcome to this week's ameu reveiwers meeting. who's here today?
Jun 17 10:00:41 <bigjools> me
Jun 17 10:00:42 <bac`> me
Jun 17 10:00:43 <allenap> me
Jun 17 10:00:43 <gmb> me
Jun 17 10:00:44 * jtv (i=0@ppp-124-120-182-45.revip2.asianet.co.th) has joined #launchpad-meeting
Jun 17 10:00:46 <adeuring> me
Jun 17 10:00:46 <salgado> me
Jun 17 10:00:55 <henninge> me
Jun 17 10:00:55 <jtv> me
Jun 17 10:01:33 <beuno> me
Jun 17 10:01:41 * intellectronica (n=tom@intellectronica.net) has joined #launchpad-meeting
Jun 17 10:01:47 <intellectronica> me
Jun 17 10:01:54 <mars> me
Jun 17 10:02:18 <flacoste> me
Jun 17 10:02:24 <barry> BjornT: ping
Jun 17 10:02:26 <barry> cprov: ping
Jun 17 10:02:37 <barry> danilos: ping
Jun 17 10:02:39 <barry> EdwinGrubbs: ping
Jun 17 10:02:46 <cprov> me
Jun 17 10:02:49 <danilos> me
Jun 17 10:02:51 <EdwinGrubbs> me
Jun 17 10:03:00 <barry> rockstar: ping
Jun 17 10:03:14 <barry> sinzui: ping
Jun 17 10:03:21 <sinzui> me
Jun 17 10:03:23 <barry> i think that's everyone
Jun 17 10:03:26 <barry> [TOPIC] agenda
Jun 17 10:03:28 <MootBot> New Topic: agenda
Jun 17 10:03:38 <barry> * Roll call
Jun 17 10:03:38 <barry> * Action items
Jun 17 10:03:38 <barry> * Mentoring update
Jun 17 10:03:38 <barry> * Peanut gallery (anything not on the agenda)
Jun 17 10:03:38 <barry> * naming of unittest methods (Camel``Case or under_scores) (henninge)
Jun 17 10:03:38 <barry> * order of assert parameters (expected, observed) (hennige)
Jun 17 10:03:38 <barry> * Driving maintainable Javascript code (intellectronica)
Jun 17 10:03:38 <barry> * Modularization and unit testing are strongly encouraged
Jun 17 10:03:38 <barry> * Refactoring code after a UI review is often a good idea
Jun 17 10:03:38 <barry> * The upper limit for JS diffs is 1600 lines (because javascript is so verbose)
Jun 17 10:03:56 <barry> [TOPIC] action items
Jun 17 10:03:57 <MootBot> New Topic: action items
Jun 17 10:04:07 <barry> i think the only one outstanding is gary's and he's away today
Jun 17 10:04:11 <barry> so...
Jun 17 10:04:16 <barry> [TOPIC] mentoring update
Jun 17 10:04:17 <MootBot> New Topic: mentoring update
Jun 17 10:04:38 <barry> i think we have just one mentored reviewer left, and noodles isn't here today
Jun 17 10:04:56 <barry> so...
Jun 17 10:05:03 <barry> [TOPIC] peanut gallery
Jun 17 10:05:04 <MootBot> New Topic: peanut gallery
Jun 17 10:05:16 <barry> * naming of unittest methods (Camel``Case or under_scores) (henninge)
Jun 17 10:05:16 <barry> * order of assert parameters (expected, observed) (hennige)
Jun 17 10:05:23 <barry> henninge: the floor is yours
Jun 17 10:05:27 <henninge> Hi
Jun 17 10:05:53 <henninge> Yesterday cprov and I ran into this discussion about how to name unittest methods.
Jun 17 10:06:15 <henninge> I have always done test_the_frobnob, while his branch had testTheFrobnob
Jun 17 10:06:41 <henninge> The argument is: Coding style for methods is CamelCase
Jun 17 10:06:50 <cprov> yes, and I've used CamelCase for most of the soyuz unittest, and it's not looking good.
Jun 17 10:07:04 <salgado> isn't the policy to use test_methodName()?
Jun 17 10:07:09 <intellectronica> i have a feeling of deja vu
Jun 17 10:07:11 * barry would greatly prefer anything make makes us more pep 8-y
Jun 17 10:07:14 <intellectronica> salgado: exactly
Jun 17 10:07:15 <mars> I thought unit_tests_were_the_exception?
Jun 17 10:07:21 <henninge> but these methods are never called directly and only appear in test runner
Jun 17 10:07:31 <henninge> output
Jun 17 10:07:31 <bigjools> salgado: +1
Jun 17 10:07:32 <jtv> this was changed a few months back, but I don't remember what it was changed to exactly.
Jun 17 10:07:44 <gmb> barry: +1
Jun 17 10:07:56 <flacoste> we already discussed that
Jun 17 10:08:04 <flacoste> the convention for unit test is:
Jun 17 10:08:21 <flacoste> test_methodName_with_particular_condition
Jun 17 10:08:33 <bac`> flacoste: that's what i remember too.
Jun 17 10:08:39 <henninge> ah, that was my faint memory, too.
Jun 17 10:08:44 <flacoste> we had this discussion like 6-8 months ago
Jun 17 10:08:46 <barry> flacoste: yes
Jun 17 10:08:58 <salgado> When testing a specific Launchpad method, a mix of PEP 8 and camel case is
Jun 17 10:08:58 <salgado> used, e.g. test_fooBarBaz()
Jun 17 10:08:58 <bigjools> is it not in the style guide?
Jun 17 10:08:59 <flacoste> barry: somebody should have updated the wiki ;-)
Jun 17 10:09:05 <salgado> https://dev.launchpad.net/TestsStyleGuide#Functional and Unit Tests
Jun 17 10:09:08 <salgado> flacoste, it's there!
Jun 17 10:09:19 <salgado> In general, you should follow Launchpad coding conventions (see PythonStyleGuide), however when naming test methods:
Jun 17 10:09:21 <salgado> ...
Jun 17 10:09:30 <flacoste> perfect!
Jun 17 10:09:32 <barry> flacoste: time machine activated!
Jun 17 10:09:36 <flacoste> anybody wants to rehash this?
Jun 17 10:09:37 * bigjools sees a rapid end to the discussion coming
Jun 17 10:09:43 <cprov> cool, problem solved.
Jun 17 10:09:50 <henninge> second item
Jun 17 10:10:26 <henninge> I have seen that LaunchpadTestCase uses assert(expected, observed)
Jun 17 10:10:36 <henninge> and have used the asserts that way.
Jun 17 10:10:52 <henninge> But I see a lot of (obseved, expected).
Jun 17 10:11:00 <barry> henninge: +1 for (expected, observed)
Jun 17 10:11:12 <henninge> barry: me, too
Jun 17 10:11:28 <bigjools> do you mean assertEqual?
Jun 17 10:11:31 <henninge> is there a guide line for that already?
Jun 17 10:11:33 <intellectronica> i don't understand
Jun 17 10:11:37 <henninge> bigjools: yeah
Jun 17 10:12:00 <henninge> oh sorry, I meant assert*(e,o)
Jun 17 10:12:26 <henninge> btw, I found out that not everybody knows about LaunchpadTestCase
Jun 17 10:12:32 <salgado> intellectronica, self.assertEquals(sut_output, 'what I expect') instead of self.assertEquals('what I expect', sut_output)
Jun 17 10:12:32 <barry> henninge: could you please update https://dev.launchpad.net/TestsStyleGuide
Jun 17 10:12:56 <intellectronica> i don't think it's worth having a policy on that. it doesn't matter
Jun 17 10:13:03 <bigjools> I don't either
Jun 17 10:13:08 <salgado> agreed
Jun 17 10:13:17 <barry> actually, i think it's better to be consistent because it's easier to debug
Jun 17 10:13:28 <bigjools> how does it make it easier?
Jun 17 10:13:45 <barry> bigjools: you have to think less when you see failing output
Jun 17 10:14:03 <henninge> yes, it's for clarity.
Jun 17 10:14:35 <intellectronica> if you want to make it really easy you can do `expected = expr \n observed = expr \n assertEqual(expected, observed)`
Jun 17 10:15:02 <henninge> intellectronica: true
Jun 17 10:15:04 <bigjools> yeah, I quite like that style
Jun 17 10:15:18 <henninge> that is fine by me, too.
Jun 17 10:15:20 <barry> intellectronica: it would be better to have that in the framework
Jun 17 10:15:29 <intellectronica> but again, i don't think we should force this. just remember that it's a possibility
Jun 17 10:15:48 <bigjools> I think encouraged but not enforced
Jun 17 10:15:53 <barry> just sayin', all things being equal, we prefer (expected, observed)
Jun 17 10:15:55 <bac`> bigjools: +1
Jun 17 10:16:00 <barry> bigjools: +1
Jun 17 10:16:10 <henninge> bigjools: +1
Jun 17 10:16:18 * bigjools is not used to this many people agreeing with him
Jun 17 10:16:38 <barry> henninge: can you capture this in the wiki?
Jun 17 10:16:45 <henninge> barry: will do
Jun 17 10:17:02 <barry> [ACTION] henninge to capture decision on assert*(expected, observed) preference in wiki
Jun 17 10:17:03 <MootBot> ACTION received: henninge to capture decision on assert*(expected, observed) preference in wiki
Jun 17 10:17:06 <barry> henninge: thanks!
Jun 17 10:17:15 <henninge> np
Jun 17 10:17:22 <barry> [TOPIC] * Driving maintainable Javascript code (intellectronica)
Jun 17 10:17:24 <MootBot> New Topic: * Driving maintainable Javascript code (intellectronica)
Jun 17 10:17:30 <barry> * Modularization and unit testing are strongly encouraged
Jun 17 10:17:30 <barry> * Refactoring code after a UI review is often a good idea
Jun 17 10:17:30 <barry> * The upper limit for JS diffs is 1600 lines (because javascript is so verbose)
Jun 17 10:17:36 <intellectronica> what a stupid title. i should be shot
Jun 17 10:17:53 <barry> intellectronica: why? because of the oxymoron? :)
Jun 17 10:18:04 * bigjools chuckles
Jun 17 10:18:18 <barry> intellectronica: the floor is yours
Jun 17 10:18:21 <intellectronica> in the last ajax swat team meeting, we rockstar and deryck led a discussion about how to produce better (more maintainable, easier to test) javascript code
Jun 17 10:18:35 <intellectronica> we made several observations:
Jun 17 10:19:11 <intellectronica> 1. code that is OO and modularized is much easier to work with than code that is very imperative and/or relies on closure too heavily
Jun 17 10:19:38 <intellectronica> 2. unit tests (aka YUI tests) are a great way to test our code. in many cases much better than integration tests
Jun 17 10:20:09 <intellectronica> 3. working on UI features it is often easier to first concentrate on completing a working UI than on code quality
Jun 17 10:20:24 <intellectronica> that's fine, but it means that we should refactor the code immediately after it passes UI review
Jun 17 10:20:57 <intellectronica> that can happen either before code review, or if not, the reviewer should encourage doing that (either as part of this branch or in a followup branch)
Jun 17 10:21:39 <intellectronica> finally, since javascript code is so much more verbose, we propose a much higher limit, in order to encourage developers to not skip these important steps
Jun 17 10:21:57 <flacoste> one of those being writing unit tests
Jun 17 10:21:59 <intellectronica> we think 1600 lines (double the limit we have for python) is reasonable
Jun 17 10:22:01 <barry> intellectronica: how can we get more assistance in making our js work correctly, and be more consistent? although everyone i've asked for help has been very helpful, i still often feel like i'm just hacking in the dark until it works
Jun 17 10:22:09 <flacoste> just unit tests can talk 2/3 of your branch size
Jun 17 10:22:46 <barry> intellectronica: when you say "javascript unit test" you're not talking about windmill, right?
Jun 17 10:22:49 <barry> or are you?
Jun 17 10:23:02 <intellectronica> barry: to some extent, we are all a bit in the dark, discovering how to best go about this. discussions in #rhinos and on the canonical-javascript mailing list are a good way to exchange ideas
Jun 17 10:23:05 <mars> barry, yuitest, not windmill
Jun 17 10:23:20 <intellectronica> barry: no. we're talking about in-page unit tests using the yuitest framework
Jun 17 10:23:26 <barry> intellectronica: fair enough :)
Jun 17 10:23:39 <intellectronica> you'll be glad to learn that they are much easier to write and execute
Jun 17 10:23:50 <barry> mars: i don't know anything about yuitest. where can i learn and/or where are good examples in our code?
Jun 17 10:24:08 <barry> intellectronica: that would make me very very happy :)
Jun 17 10:24:08 <flacoste> lazr-js/src/*/tests
Jun 17 10:24:11 <sinzui> where are these unit tests in our tree?
Jun 17 10:24:20 <intellectronica> barry: see lib/canonical/launchpad/javascript/soyuz/tests for example
Jun 17 10:24:24 <mars> barry, right now the best examples are in lazr-js itself, but others will be landing examples in launchpad soon
Jun 17 10:24:25 <flacoste> <...>/javascript/*/tests
Jun 17 10:24:32 <intellectronica> also all widgets in lazr-js have tests
Jun 17 10:24:34 <flacoste> currently,only the soyuz module has some
Jun 17 10:24:41 <flacoste> yep lazr-js/src/*/tests
Jun 17 10:24:50 <barry> cool, thanks, i will be looking at those for the branch i'm currently working on
Jun 17 10:25:03 <flacoste> unit tests are really "unit" tests
Jun 17 10:25:06 <flacoste> no Launchpad
Jun 17 10:25:12 <flacoste> only DOM and you code
Jun 17 10:25:23 <flacoste> and it's an self-created DOM
Jun 17 10:25:24 <mars> barry, btw, the test infrastructure is open for hacking and improvement. Please feel free to stop the pain :)
Jun 17 10:25:26 <sinzui> So I believe lp_dynamic_dom_updater.js is the only Launchpad test in our tree
Jun 17 10:25:39 <flacoste> in lauchpad, yes
Jun 17 10:25:44 <flacoste> lazr-js has tons of them
Jun 17 10:25:46 <intellectronica> so you still need to do some integration testing, but the advantage is that they encourage you to do a good job on modularizing your code
Jun 17 10:26:07 <barry> mars: i will as soon as i can localize it. right now, it's mostly a full-body numbing throb. :)
Jun 17 10:27:30 <barry> i think the other problem is that anything touching js just takes WAY more time to develop. i don't think it's entirely the fault of not being as familiar with js. it's a function of the ui 90/10 rule and the fact that yui documentation is lousy
Jun 17 10:27:52 * barry knows he's just ranting
Jun 17 10:28:18 <intellectronica> barry: that, and the fact that we're all still learning. i bet in a couple of years we'll all be js/yui wizards. it takes time to build competency
Jun 17 10:28:40 <mars> barry, thankfully we have started to work on that. intellectronica's process improvements are an important step towards speeding things up
Jun 17 10:28:51 <barry> intellectronica: true. though, we need to keep that in mind when estimating story points and such
Jun 17 10:29:01 <sinzui> intellectronica: so your saying in a couple of year javascript will require less keystrokes to code?
Jun 17 10:29:16 <barry> mars: yes, and they are greatly appreciated. our velocity will certainly improve over time
Jun 17 10:29:22 <mars> sinzui, yes, actually
Jun 17 10:29:26 * beuno yells from the back "FASTER! FASTER!"
Jun 17 10:29:28 <intellectronica> sinzui: i'm sure in a couple of years you will have written enough gedit plugins to automate most of the crud :)
Jun 17 10:29:48 <sinzui> I guess the soyuz team won't notice since their class names exceed the line limit
Jun 17 10:30:09 * barry mumbles "dabbrev"
Jun 17 10:30:18 * sinzui pines for Javascript 1.7/ECMA ??
Jun 17 10:30:37 <bigjools> at least we have tests!
Jun 17 10:30:41 <mars> sinzui, you want ECMA 3.1 :)
Jun 17 10:30:45 <intellectronica> i think we digress. maybe we should start a js coder support group for ranting...
Jun 17 10:30:50 <intellectronica> is everyone cool with the increase in diff limit (and understands why it's necessary)?
Jun 17 10:30:58 <barry> intellectronica: yes :)
Jun 17 10:31:04 <sinzui> I think YUI looks to Java for inspirtation where are Javascript 1.7 looks to Python
Jun 17 10:31:28 <mars> sinzui, not the cloaked OO pining of 4.X ;)
Jun 17 10:31:31 <barry> intellectronica: it does make it more difficult to review, but it does make sense.
Jun 17 10:31:58 <intellectronica> barry: well, you don't really need to review the block delimiters and the uber-verbose documentation
Jun 17 10:32:01 <mars> intellectronica, +1
Jun 17 10:32:06 <barry> intellectronica: true
Jun 17 10:32:25 <barry> intellectronica: i'm certainly up for an experiment here. if it's too painful we can address it again
Jun 17 10:33:10 <intellectronica> i think i'm done, unless anyone has anything to add
Jun 17 10:33:36 <intellectronica> and no, "javascript sucks" isn't a valid item. we already know that :-/
Jun 17 10:33:44 <barry> intellectronica: thanks. can you send a message to launchpad@ about the length limit?
Jun 17 10:33:46 <mars> :P
Jun 17 10:33:55 <intellectronica> barry: will do
Jun 17 10:34:14 <barry> [ACTION] intellectronica to email launchpad@ about js branch length limit increase
Jun 17 10:34:15 <MootBot> ACTION received: intellectronica to email launchpad@ about js branch length limit increase
Jun 17 10:34:16 <sinzui> How are we increasing our JS reviewers?
Jun 17 10:34:35 <intellectronica> sinzui: i think by now pretty much everyone on the team does js reviews
Jun 17 10:34:44 <mars> sinzui, formally?
Jun 17 10:34:48 <bigjools> which team?
Jun 17 10:35:02 <intellectronica> bigjools: reviewers team (which is almost the same as the developers team)
Jun 17 10:35:22 <bigjools> intellectronica: I don't review JS then
Jun 17 10:35:29 <mars> sinzui, because js reviews are open to all, we leave it up to the reviewer's discretion
Jun 17 10:35:35 <barry> right. we're asking all reviewers to do js reviews, but ask the swat team for help when needed
Jun 17 10:35:36 <sinzui> mars: I know a lot about javascript, but I do not consider myself a review because I do not know much about YUI, YUI unit tests, and windmill
Jun 17 10:35:38 <intellectronica> bigjools: well, time to start, then :)
Jun 17 10:35:47 <bigjools> intellectronica: I need to write some js myself first :)
Jun 17 10:35:56 <salgado> same with me
Jun 17 10:36:03 <bac`> me2
Jun 17 10:36:04 <intellectronica> bigjools: yes, i guess that's a prerequisite
Jun 17 10:36:11 <barry> one problem with this though is that there's little consistency in our js style
Jun 17 10:36:21 <mars> sinzui, right. Depends on the review. If it is a one-line logic bug, then you don't need a JS mentor call
Jun 17 10:36:29 <barry> for example, every branch i've written has had different recommendations during js review
Jun 17 10:36:31 <intellectronica> barry: anything in particular you've noticed?
Jun 17 10:37:02 <mars> sinzui, but if you desire a second opinion, then give someone on the AJAX team a shout
Jun 17 10:37:27 <barry> intellectronica: little things mostly, like where to stash objects, anim-reversing
Jun 17 10:37:42 <barry> intellectronica: i think of it more as refinement rather than conflicts
Jun 17 10:37:51 <flacoste> yeah
Jun 17 10:37:56 <flacoste> that pattern are not clear yet
Jun 17 10:38:05 <barry> mars: can you remind us who is on the "ajax team"?
Jun 17 10:38:07 <mars> anim-reversing is about learning, and weak docs (my fault)
Jun 17 10:38:13 <flacoste> so i think it's normal to get divergent opinion on these aspects
Jun 17 10:38:40 <barry> mars: no, i think flacoste has it right. we're learning as we go, so consistency suffers for now. we're just building up tech debt we know we'll have to pay off later
Jun 17 10:38:47 * barry sees no other way around that
Jun 17 10:38:56 <flacoste> barry: EdwinGrubbs, mars, intellectronica, jtv, flacoste, rockstar
Jun 17 10:39:01 <flacoste> barry: and noodles
Jun 17 10:39:14 * mars can't type today
Jun 17 10:39:15 <flacoste> and danilos - by virtue of having attended the Berlin sprint
Jun 17 10:39:33 <barry> flacoste: thanks. we should capture that on a wiki page, if it isn't already
Jun 17 10:39:44 <danilos> barry: I think it is
Jun 17 10:39:44 <mars> barry, it is
Jun 17 10:39:49 <barry> cool
Jun 17 10:39:52 <barry> thanks
Jun 17 10:39:54 <intellectronica> barry: deryck too knows a lot about js and yui and can help, b.t.w
Jun 17 10:40:01 <mars> barry, just check the 'skills' column on the current reviewers page
Jun 17 10:40:20 <danilos> barry: https://dev.launchpad.net/ReviewerSchedule
Jun 17 10:40:36 <barry> as i said, everytime i've asked someone for some js help, y'all have been very nice and very helpful. thanks!
Jun 17 10:40:36 <mars> s/skills/specialities/
Jun 17 10:41:11 <barry> we have 5 minutes left. is there anything else to talk about today?
Jun 17 10:41:28 <flacoste> yes
Jun 17 10:41:29 <flacoste> i have
Jun 17 10:41:46 <flacoste> i'd like to get an update on the JS testing experiment
Jun 17 10:41:51 <flacoste> anyone tried it out in the last week?
Jun 17 10:42:24 <barry> nope
Jun 17 10:42:45 <mars> no, no JS landings :(
Jun 17 10:42:56 <flacoste> really?
Jun 17 10:42:58 <flacoste> ok
Jun 17 10:43:03 <intellectronica> a better question is: anyone been through review with a js branch which hasn't gone through testing?
Jun 17 10:43:15 <flacoste> yes, that's the proper way to phrase the question?
Jun 17 10:43:16 <flacoste> !
Jun 17 10:43:33 <barry> no, but i have a js branch going into review hopefully today
Jun 17 10:43:41 <flacoste> aha!
Jun 17 10:43:51 <barry> i guess i can be a guinea pig :)
Jun 17 10:44:10 <flacoste> what about the timeline branches that Edwin landed last week?
Jun 17 10:45:52 <mars> flacoste, the easiest way to push back on this may be to have reviewer's ask "Has it been tested on all browsers?"
Jun 17 10:46:04 <barry> guess not. we're outta time, so shall we call it a day?
Jun 17 10:46:10 <flacoste> sure
Jun 17 10:46:25 <barry> flacoste: let's ask again next week!
Jun 17 10:46:27 <barry> #endmeeting
