Code review comment for lp:~henninge/launchpad/bug-467079

Hi Henning.

  review needs-fixing

Your view and CSS changes are good. The narrative of the story needs
revising. It is not a story. The narrative must explain what is seen and
happens from the users perspective. *We* care about contracts, and if
this test ware really about what *we* want to create, we could
instantiate the view, call render() and verify the markup much easier in
a view test.

I do not see how this test can pass. It checks for ids that do not exist
since the columns table-column properties were replace with simple CSS.

On Tue, 2009-11-17 at 17:23 +0000, Henning Eggers wrote:
> === modified file 'lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt'
> --- lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-09-21 14:48:17 +0000
> +++ lib/lp/registry/stories/launchpad-root/xx-featuredprojects.txt 2009-11-17 17:21:11 +0000
> @@ -24,21 +24,32 @@
> >>> print extract_text(featured.h2)
> Featured projects
>
> -We show the featured projects in our sample data, the "project of the day" is
> -"GNOME" and it is listed first:
> +We show the featured projects in our sample data. The "project of the day"
> +is "GNOME" and it is listed separately:

Any user can see the Launchpad featured projects. The project of the day
is listed separately.

> - >>> for link in featured.findAll('a'):
> - ... print extract_text(link)
> + >>> top_project = find_tag_by_id(featured, 'homepage-featured-top-title')
> + >>> print extract_text(top_project)
> GNOME
> +
> +These are the projects in the left column.

He see's content in a left column.

--but I cannot see how this will work because the template does not have
these ids, we let the stylesheet do the layout. There is a single list
that we need to test. If the layout is changed in the CSS, we do not
need to worry about the test breaking. This was one of the 3.0 UI
goals--no brittle tests that verifies markup. While this does not verify
the markup, it still feels like it is trying to force a two column
table.

> + >>> def print_column(column_id):
> + ... column = find_tag_by_id(featured, column_id)
> + ... for link in column.findAll('a'):
> + ... print extract_text(link)
> + >>> print_column('homepage-featured-left')
> Gnome Applets
> Bazaar
> Mozilla Firefox
> Gentoo
> +
> +These are the projects in the right column.

And content in a right column.

-- but I know that this test will fail

> + >>> print_column('homepage-featured-right')
> GNOME Terminal
> the Mozilla Project
> Mozilla Thunderbird
> Ubuntu
> - Browse all ... projects
>
> == Adding a featured project ==
>
> @@ -88,19 +99,27 @@
>
> >>> anon_browser.open('http://launchpad.dev/')
> >>> featured = find_tag_by_id(anon_browser.contents, 'homepage-featured')
> - >>> for link in featured.findAll('a'):
> - ... print extract_text(link)
> + >>> top_project = find_tag_by_id(featured, 'homepage-featured-top-title')
> + >>> print extract_text(top_project)
> Gentoo

I am not sure 'odd' is important here if we change the css, to be 3
column or 1 column it will not be odd. As I said above, the ids you are
testing do not exist. There is a single list of content that the
stylesheet controls. All we need to verify here is that the user *sees*
the changed list.

> +These are the projects in the left column. Because the number of projects in
> +the columns is odd, the first column is longer.
> + >>> print_column('homepage-featured-left')
> Apache
> Gnome Applets
> Bazaar
> Mozilla Firefox
> GNOME
> +
> +These are the projects in the right column.
> +
> + >>> print_column('homepage-featured-right')
> GNOME Terminal
> the Mozilla Project
> Mozilla Thunderbird
> Ubuntu
> - Browse all ... projects

--

__C U R T I S C. H O V E Y_______
Guilty of stealing everything I am.