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

> 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

Indeed I noticed that. I just think that each of your test cases are doing too much in one go. Hence I wanted the split.

> > 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.

Helps, but not a cure. We cannot assume the machine isn't slow, we don't want failures because of it.

> I can borrow the same idea. If you don't like the 'sleep 2' :)

Please use the tmpwindow lib. It's a bit safer.

>
> >
> > 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.

Ok, in which case, it should be part of "setup" - or maybe "startup" and then being sure that "teardown" closes all windows created during tests.

> > @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)

Oh you use it in the shutdown, I missed that. Apologies.

> > + 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

Ok.

> 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;)

I don't care what test is happening, until it fails, in which case the error tells you what test failed. These comments are nice, but will litter log files, so I prefer to not have them.