Code review comment for lp:~dyams/unity-2d/update-pips

> About your test suite: this makes a big change so it needs several tests.
>
> First off can you please add a good test-description as a comment to the top
> of each test-case, like
> https://bazaar.launchpad.net/~unity-2d-team/unity-
> 2d/trunk/view/head:/tests/launcher/autohide_show_tests.rb
>
Done

> I'd also prefer you split the test cases just a little, so you get tests for:
> 1. One app on this workspace only
> 2. One app on other workspace only
> 3. Two apps on this workspace
> 4. Two apps, both on other workspace
> 5. Two apps, one on this workspace, one on other
> 6. Three apps all on this workspace
> 7. Three apps, all but one on this workspace
> I know it's a lot, but these are all the possible cases that I see your code
> influencing.
Actually current test suite considers them all. By Two apps you mean an app with two windows not two different applications. no?
Well, current tests already have separate tests for app with only one window and app with more than one window.
When testing with more than one window, i have also tested by moving each window to different workspace individually.
1) Two windows on current workspace
2) One window on current workspace and another window on different workspace
3) Both windows on different workspace than current

>
>
> This code isn't perfect:
> + Timeout.timeout(3){XDo::XWindow.wait_for_window('Calculator')}
> When you've one calculator open, this will return immediately. It is possible
> then that
> + return XDo::XWindow.from_active
> will return the first calculator XId.
>
There is a 'sleep 2' in between the two statements, which helped me to address the issue you have mentioned.

> This is the reason I was using terminals with unique titlebars in other tests.
> At least then you can uniquely identify the window. Hacky, I know. If you know
> of a way to run a program & get the XId of the window reliably, I'd love to
> hear it! (via PID is a good idea, except for Terminal:) )
I can borrow the same idea. If you don't like the 'sleep 2' :)

>
> Have a look at tests/misc/lib/tmpwindow.rb (in latest trunk). It also has a
> cleanup method so that if a test fails, you don't have non-closed windows to
> worry about. It will mean you can remove "kill" statements from the start of
> test-cases. Call the cleanup method in the teardown, it's
>
No, this kill statements are not for cleaning the residues of the test case. Instead it is a prerequisite. If there are more than one window then pip image is different than when there is only one window. So we need to ensure there is no running windows of the test application.

>
> @number_of_workspaces - this can be a local variable, so no @.
>
If I remove that @ I got this error
.update_pips_tests.rb:62: undefined local variable or method `number_of_workspaces' for #<Class:0xb6e7cbf4> (NameError)

> + width = %x{xrandr --current | grep \'\*+\' | uniq | awk \'{print $1}\' | cut
> -d \'x\' -f1}
> It would be great if you added this to the XDoTool Ruby bindings for everyone
> to use. Parsing the output with Ruby would probably be quicker?? :)
>
This is part of multi-monitor stuff, which is totally removed from this MR already
>
> Small "puts" comments in your test suite are not necessary either. And there's
> some extra blank lines.
I have removed 'puts'. I thought they would inform what the test case is doing while we are waiting for them to finish. Unfortunately they did serve the purpose it seems;)