10/05/14 17:22, intrigeri wrote:
> Hi,
>
> anonym wrote (08 May 2014 15:52:25 GMT) :
>> intrigeri, please review and merge test/6559-adapt-test-suite-for-Wheezy
>> into devel.
>
> Here's my review. Test suite run in progress, I'll inspect results and
> report back later (likely tomorrow).
>
> Commit a767bfb ("Add I2P repo to the APT sources test") feels wrong:
> adding this repo to our sources is meant to be temporary, and not
> intended to go into a release (see #6731). So, I don't think we should
> let our test suite pretend it's OK. Instead, this failure should serve
> as a reminder that we don't want to keep things this way.
Ok, reverted.
OT: could you just clarify; why are we gonna stop using kytv's repo? At
least that's what I interpret the ticket as, that we're gonna go back to
manually importing the packages. Is it for providing sources?
> Regarding the removal of test suite steps that look at notifications:
>
> 1. It seems to work on my hardware + current Internet connection (up
> to the features I've already tested), and it certainly works on
> yours. But IIRC these steps were added to avoid the notifications
> breaking things, when they appear concurrently with another app's
> window that we want to find on the screen. I'm not sure if we can
> confident that this won't happen anymore on slower hardware, or
> Internet connection. I don't object to merging this as is, but
> I'm a bit afraid this bites us in the future. We'll see.
Unfortunately the hardware argument goes both ways -- on slow hardware
Sikuli might not find the notifications before they have disappeared
thanks to GNOME3's greatly reduced timeout (no matter what's set
manually as the timeout).
The other problem is when we try to close them -- they may disappear
right before the click, and then the click may shift application focus,
or click on something else (perhaps the X-button of a window, which
often is in the notification areas vicinity). BTW, this could only
happen in "I see and close the Unsafe Browser start notification" of the
removed tests. Luckily the reduced timeout is our friend here since we
then can just waitVanish instead of clicking.
> 2. These steps were also useful to... actually test that we're
> displaying these notifications. In the context of the test suite,
> they are annoying, but for actual users, they are features.
> We have dropped tests such as "I see a notification that Unsafe
> browser is starting" from our manual test suite when the
> automated test suite implemented it. And now, we're dropping this
> test from the automated test suite too. I've not carefully
> checked if other similar tests were removed. That looks like
> a regression to me. What do you think?
ACK. I'll bring it back tomorrow with some needed modifications.
> It's not clear to me what commit 6cecfe6 ("Do not purge bind9-host's
> dependencies") has to do in this branch. The commit itself makes
> sense, so I don't mind, but I felt like raising it to make sure it
> wasn't a mistake.
Sorry for being unclear. Without that commit we're left with no
/usr/bin/host in Wheezy, which firewall_checks.feature depends on. This
is why I said the images to be tested must be built from this branch.
> I'm not convinced by your rewriting (17f2a98) of the commit message
> I wrote in 1c5991c, losing useful info along the way, but oh well, who
> cares. It feels sub-optimal that the erroneous commit message for
> cdbee937 was not fixed, while you were at rewriting history. Same for
> 4c7beba + its revert commit, that could have been dropped. No big
> deal, probably. </nitpicking> (or not, I've more!)
Note that I didn't rewrite *my* branch's history, in case you had
already started looking at it. Rewriting it now may be too late, unless
you dare relying on a `git diff` between your local branch and a force
pushed one (if the diff is empty up until the commit that corresponds to
the HEAD of your local branch, all should be ok). Your call.
> I think that the error message raised in /^process "([^"]+)" is
> running within (\d+) seconds$/ should be different from the one in
> /^process "([^"]+)" is running$/, and it should convey the "N seconds"
> information. What do you think?
I agree, now fixed.
> The commit message ("Clean up.") for 4cd0017f does not tell me why
> short command-line options disappear.
As you can see the short options do nothing and never did. My only
explanation is that I committed them by mistake when I initially
getopt:ified the script (commit e273f40).
> I'm not 100% convinced by the way @skip works. [...]
> I would rather see the entire @skip (or whatever it's called in the
> end) thing be reverted in test/6559-adapt-test-suite-for-Wheezy,
> un-reverted in its own branch, and considered later, so that this does
> not impact the merge-ability of the high-priority branch we're
> working on.
Done. See new thread "How to deal with tests expected to fail?" for more
on this. I'll open a ticket as soon as it hits the archive so I can link
to my conclusions. IMHO this can wait until after 1.1 since we have more
pressing matters until then (at least).
> Oh, and congrats for the option to make Sikuli rety on FindFailed!
As you probably have understood it's a quite useful feature for updating
many images, even if it dt catch all situations. :)
> I don't understand the complexity added by commit bc6805ec, and the
> commit message tells me nothing about it. Have you seen actual cases
> when TailsData was *not* unlocked+mounted, after t-p-s exited?
That step is also used in scenarios when t-p-s isn't run at all, and
persistence is disabled, see e.g. "Booting Tails from a USB drive with a
disabled persistent partition".
Cheers!
