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.
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.
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?
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.
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!)
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?
The commit message ("Clean up.") for 4cd0017f does not tell me why
short command-line options disappear.
I'm not 100% convinced by the way @skip works. I understand it can
greatly speed up running the test suite, which is especially useful
when working on it. But IMO, that's not the general case, and I'm
quite sure it should not be treated this way by default (e.g.
at release time). In the test frameworks I'm used to, here's how the
TODO (known broken) tests work: a test case flagged as such is run,
and then: if it fails, then it's result is ignored, and does not
affect the overall result of the test suite; if it passes, then that's
considered (maybe surprisingly) as a failure, which is actually
logical since it most likely indicates that the test suite is
outdated, or buggy. Without some similar logic, I'm concerned that we
never think of unflagging tests marked as @skip, when we fix the
corresponding bugs. I realize that the current implementation (--tags
~@skip) is trivial, and that implementing the logic I'm suggesting
above would likely be more involved (right?). So, how about 1.
renaming @skip to @todo, and 2. supporting a --skip-todo *opt-in*
switch, that does what you have implemented, and that we would *not*
use without very good reasons, and certainly not at release testing
time. What do you think?
Also, does "--tags ~@skip" conflict with any other usage of --tags
that developers may find useful to (somehow) pass on the cucumber
command-line themselves? Maybe CUCUMBER_OPTS should not lose
a potential existing value of itself, when initialized?
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.
Oh, and congrats for the option to make Sikuli rety on FindFailed!
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?
Overall, great job!
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
