See mailing list archives
for discussion on individual patches.
msg18604 (view) |
Author: bfrk |
Date: 2015-06-22.23:55:56 |
|
This is obviously something to be discussed, so not yet for screened. Note
that the network tests all succeed now (for me) and have been trimmed down
(by someone else, some time ago) so that they no longer use unreasonable
amounts of resources.
1 patch for repository http://darcs.net/screened:
patch b51475db75cf8a111ab8ae76f3967f9505defd5f
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date: Mon Jun 22 09:37:54 CEST 2015
* run network tests by default
Attachments
|
msg18721 (view) |
Author: ganesh |
Date: 2015-08-03.16:49:49 |
|
I'm not too sure what I think about this.
In general it seems like a reasonable idea to improve our coverage.
In practice I mostly run the tests when I have an unreliable or
unavailable network connection, so either I'll end up having to
reorganise my working practices a bit, or I'll continue not running
these tests even if other people are doing so by default.
In principle tests that depend on external data are a bad idea because
that data might change and destablise the tests, though I think these
tests just rely on there being a repo at http://darcs.net which ought to
be pretty stable.
|
msg19053 (view) |
Author: bfrk |
Date: 2016-03-07.08:56:00 |
|
I am still very much in favour of this change. It is of utmost
importance to prevent regressions, especially with the often tricky
network code. Your practice of running tests when you have no reliable
internet connection is, perhaps, not too common? And would adding the
--network=no in that case really be so inconvenient?
|
msg19054 (view) |
Author: ganesh |
Date: 2016-03-07.19:54:42 |
|
OK, I can live with it.
I'm happy to run with `--network=no` if necessary. My main concern is that it's often
distracting for other people if the default set of tests fail on main, and if I break
those tests I might push without realising. That said I guess it's still better than me
breaking the tests and noone ever noticing.
For me ideally these tests would be self-contained, e.g. run a webserver themselves and
connect to that, but unless/until someone does that it seems worth living with the
imperfect situation.
|
msg19060 (view) |
Author: bfrk |
Date: 2016-03-09.03:04:21 |
|
I have an idea how to solve the dilemma. Inside the network test shell
scripts (there are not so many of them, actually) we first test whether
the affected host(s) are reachable; perhaps even check with curl that
the URLs are valid. If this fails, then we exit with the standard 'skip
this test' code. IIUC this will print a message that the test was
skipped (can we also give a reason? I think so, must check).
|
msg19064 (view) |
Author: gh |
Date: 2016-03-09.13:20:10 |
|
I would also like to have network tests running by default but all
being self-contained. For the ones that really need external data,
check in 1 second if this remote data is available and skip if not.
This makes me think of something that always bugged me in our
testsuite: test are not announced before they are run...
Is it something that tasty solves ? (wrt test-framework)
Guillaume
|
msg19070 (view) |
Author: bfrk |
Date: 2016-03-09.18:08:57 |
|
An updated patch bundle that implements the solution I proposed. Network
tests suceed here, whether network is turned on or off. It is a bit
unfortunate that the harness does not report the number of skipped tests,
nor does it support printing a message indicating why a certain test was
skipped.
4 patches for repository http://darcs.net/screened:
patch b51475db75cf8a111ab8ae76f3967f9505defd5f
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date: Mon Jun 22 09:37:54 CEST 2015
* run network tests by default
patch e10918d16c3edc12c570ae2a8c81eaa276ab7a72
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date: Wed Mar 9 16:37:41 CET 2016
* skip http network tests when server does not respond
The shell test lib has a new function check_remote_http that uses curl to
test if a http server responds at the given URL and exit with the skip code
if not. All network tests that use http have been updated to make this check
first.
patch ca5f18323a4068b2c996f6149c07290d1dd046ed
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date: Wed Mar 9 16:42:41 CET 2016
* fix ssh network tests so they work in the test harness
These tests are still configurable by the user with the REMOTE and
REMOTE_DIR variables. However, if the variables are empty, we choose a
sensible default for them that should work on most system setups, namely
REMOTE=$(whoami)@localhost and REMOTE_DIR=/tmp/darcs-ssh-test, even if the
network is down.
patch 0145c68429e239781cdcec9652bd8e307352a11c
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date: Wed Mar 9 16:50:46 CET 2016
* made network ssh tests more robust by passing --skip-long-comment
When using the default REMOTE $(whoami)@localhost, the default settings of
the user that runs the tests are used on the remote side, instead of
artificial ones as used when running darcs locally. This means we need to
cope with non-default settings, and one of these is to restore the old darcs
behavior of prompting for a long comment when recording. Passing
--skip-long-comment overrides such a setting.
(This patch also simplifies multi-line quotes and breaks some long lines.)
Attachments
|
msg19075 (view) |
Author: bfrk |
Date: 2016-03-12.20:37:32 |
|
Ganesh, are you okay with this?
|
msg19078 (view) |
Author: ganesh |
Date: 2016-03-13.16:40:51 |
|
Yes, I'm fine with it.
|
msg19082 (view) |
Author: bfrk |
Date: 2016-03-15.10:45:16 |
|
Ok, I'm screening this bundle then.
|
msg19092 (view) |
Author: gh |
Date: 2016-03-17.23:40:29 |
|
Running the tests against latest Darcs HEAD I get the error:
ssh: connect to host localhost port 22: Connection refused
in the tests:
* network/ssh.sh
* network/issue2090-transfer-mode.sh
Can't these tests be skipped when a local ssh server is not present?
|
msg19095 (view) |
Author: gh |
Date: 2016-03-18.17:14:58 |
|
OK I'm accepting the bundle with patch1470
|
msg19205 (view) |
Author: ganesh |
Date: 2016-05-04.17:45:34 |
|
The network tests are now causing trouble in other test
environments: https://github.com/fpco/stackage/issues/1399
Not sure what to do, but I think there's some value in 'cabal test'
being something that other people can run reliably. That might tip
the balance against running the network tests by default.
|
msg19206 (view) |
Author: simon |
Date: 2016-05-04.18:04:51 |
|
Anything that makes running tests more fragile adds friction for development
and packaging. I don't think requiring network access for the tests which run
by default is ok.
|
msg19208 (view) |
Author: gh |
Date: 2016-05-05.14:40:15 |
|
OK I agree with Ganesh' comment (cabal test should be more reliable).
Judging from the github discussion, the failing test is expecting the
ssh connection to fail and emit a precise error message. This is
probably too risky, for network reasons and also for the fact that we
depend on third-party software.
I'm sending a patch for Darcs 2.12.1 that disables network tests by default.
|
msg19209 (view) |
Author: bfrk |
Date: 2016-05-05.17:12:12 |
|
I am not sure you draw the right conclusions from the available data.
First, the offending test is not inside tests/network:
ben@sarun[3]: .../darcs/screened > find -name issue1932-colon-breaks-add.sh
./tests/issue1932-colon-breaks-add.sh
so disabling the network tests will be of no use here. Since apparently
this test does (try to) connect via ssh, the first thing to do would be
to move the test to tests/network.
Second, there are mechanisms in place inside the tests/network directory
that detect whether connecting via ssh works at all and skip the test if
not. These are at the end of tests/network/sshlib:
# test if we can connect via ssh, otherwise skip test
${SSH} ${REMOTE} true || exit 200
Perhaps sourcing this file like the other network tests do will already
be enough to skip this particular test.
Third, the test as written relies on the very particular wording of the
error message from the ssh library. This is doomed to fail sooner or
later. The test should be re-written to be (much) more tolerant wrt the
exact wording of this message.
|
msg19210 (view) |
Author: gh |
Date: 2016-05-05.17:32:20 |
|
Ben, in the Github thread there are two reports: juhp's one which is
up-to-date and peti's one which is outdated (darcs 2.10.3), and
./tests/network/issue1932-remote.sh was not split yet from
./tests/issue1932-colon-breaks-add.sh.
You're right that we could add sshlib to this script anyway, but just in
case, I would disable network tests in the release branch and leave them
in HEAD.
... and now that I read the Github thread, I don't understand juhp's error.
|
msg19215 (view) |
Author: bfrk |
Date: 2016-05-06.19:29:24 |
|
> disable network tests in the release branch and leave them in HEAD.
Hm, yes, that sounds like a very reasonable compromise, go ahead!
We should still try to fix the network tests so that even when ssh (or
whatever else is needed) isn't installed, the test is just skipped.
|
msg19239 (view) |
Author: simon |
Date: 2016-08-30.00:21:12 |
|
Related: http://bugs.darcs.net/issue2504
|
|
Date |
User |
Action |
Args |
2015-06-22 23:55:56 | bfrk | create | |
2015-08-03 16:49:50 | ganesh | set | status: needs-screening -> in-discussion messages:
+ msg18721 |
2016-03-07 08:56:01 | bfrk | set | messages:
+ msg19053 |
2016-03-07 19:54:43 | ganesh | set | messages:
+ msg19054 |
2016-03-09 03:04:24 | bfrk | set | messages:
+ msg19060 |
2016-03-09 13:20:10 | gh | set | messages:
+ msg19064 |
2016-03-09 18:08:58 | bfrk | set | files:
+ patch-preview.txt, run-network-tests-by-default.dpatch, unnamed messages:
+ msg19070 |
2016-03-12 20:37:33 | bfrk | set | messages:
+ msg19075 |
2016-03-13 16:40:52 | ganesh | set | messages:
+ msg19078 |
2016-03-15 10:45:16 | bfrk | set | status: in-discussion -> needs-review messages:
+ msg19082 |
2016-03-17 23:40:30 | gh | set | messages:
+ msg19092 |
2016-03-18 17:14:58 | gh | set | status: needs-review -> accepted messages:
+ msg19095 |
2016-05-04 17:45:35 | ganesh | set | messages:
+ msg19205 |
2016-05-04 18:04:51 | simon | set | messages:
+ msg19206 |
2016-05-05 14:40:15 | gh | set | messages:
+ msg19208 |
2016-05-05 17:12:13 | bfrk | set | messages:
+ msg19209 |
2016-05-05 17:32:21 | gh | set | messages:
+ msg19210 |
2016-05-06 19:29:24 | bfrk | set | messages:
+ msg19215 |
2016-08-30 00:21:12 | simon | set | messages:
+ msg19239 |