darcs

Patch 1378 run network tests by default

Title run network tests by default
Superseder Nosy List bfrk
Related Issues
Status accepted Assigned To
Milestone

Created on 2015-06-22.23:55:56 by bfrk, last changed 2016-08-30.00:21:12 by simon.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bfrk, 2015-06-22.23:55:56 text/x-darcs-patch
patch-preview.txt bfrk, 2016-03-09.18:08:57 text/x-darcs-patch
run-network-tests-by-default.dpatch bfrk, 2015-06-22.23:55:56 application/x-darcs-patch
run-network-tests-by-default.dpatch bfrk, 2016-03-09.18:08:57 application/x-darcs-patch
unnamed bfrk, 2015-06-22.23:55:56
unnamed bfrk, 2016-03-09.18:08:57 text/plain
See mailing list archives for discussion on individual patches.
Messages
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
History
Date User Action Args
2015-06-22 23:55:56bfrkcreate
2015-08-03 16:49:50ganeshsetstatus: needs-screening -> in-discussion
messages: + msg18721
2016-03-07 08:56:01bfrksetmessages: + msg19053
2016-03-07 19:54:43ganeshsetmessages: + msg19054
2016-03-09 03:04:24bfrksetmessages: + msg19060
2016-03-09 13:20:10ghsetmessages: + msg19064
2016-03-09 18:08:58bfrksetfiles: + patch-preview.txt, run-network-tests-by-default.dpatch, unnamed
messages: + msg19070
2016-03-12 20:37:33bfrksetmessages: + msg19075
2016-03-13 16:40:52ganeshsetmessages: + msg19078
2016-03-15 10:45:16bfrksetstatus: in-discussion -> needs-review
messages: + msg19082
2016-03-17 23:40:30ghsetmessages: + msg19092
2016-03-18 17:14:58ghsetstatus: needs-review -> accepted
messages: + msg19095
2016-05-04 17:45:35ganeshsetmessages: + msg19205
2016-05-04 18:04:51simonsetmessages: + msg19206
2016-05-05 14:40:15ghsetmessages: + msg19208
2016-05-05 17:12:13bfrksetmessages: + msg19209
2016-05-05 17:32:21ghsetmessages: + msg19210
2016-05-06 19:29:24bfrksetmessages: + msg19215
2016-08-30 00:21:12simonsetmessages: + msg19239