Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
tests, integration-tests: port network interface test to spread #1367
Conversation
fgimenez
added some commits
Jun 21, 2016
fgimenez
referenced this pull request
Jun 22, 2016
Merged
tests, integration-tests: port interfaces cli to spread #1382
|
+1 |
|
The code looks good but tests fail:
|
|
+1, except tests fail now |
|
@zyga yep, a problem with the snap path, fixed. Thanks! |
|
autopkgtest is failing. |
|
retest this please |
|
@kyrofa yep mmm the failing test shouldn't be affected by these changes, the snaps have been copied, not moved, and are not changed, i've retriggered them to confirm the error |
niemeyer
reviewed
Jun 22, 2016
| @@ -0,0 +1,85 @@ | ||
| +summary: | |
niemeyer
Jun 22, 2016
•
Contributor
The summary should really be a be a one-line short summary. Something like "Ensure network interface works."
The content below is nice, though, and should go under:
details: |
The network interface ...
niemeyer
reviewed
Jun 22, 2016
| + SERVICE_FILE: "./service.sh" | ||
| + SERVICE_NAME: "test-service" | ||
| + | ||
| +kill-wait: 1m |
niemeyer
Jun 22, 2016
Contributor
Let's please not have 1m kill timeout on anything unless it's extremely trivial. Otherwise it's way too short.. any sort of network instability would kill the test, as has happened before in the other case we discussed yesterday.
Also note that the field is kill-timeout not kill-wait.
niemeyer
reviewed
Jun 22, 2016
| +environment: | ||
| + SNAP_NAME: network-consumer | ||
| + SNAP_FILE: "./$[SNAP_NAME]_1.0_all.snap" | ||
| + PLUG: network |
niemeyer
Jun 22, 2016
Contributor
Can we please have the word "network" inlined instead of this variable? This is a constant really, and is used in contexts which are not about a plug, specifically. So it's obscuring the test rather than helping out.
fgimenez
Jun 22, 2016
Contributor
Done, this could allow us to share common bits between all the interface tests, but now it isn't useful at all
niemeyer
reviewed
Jun 22, 2016
| +prepare: | | ||
| + echo "Given a snap declaring the $PLUG plug is installed" | ||
| + snapbuild ../lib/snaps/$SNAP_NAME . | ||
| + sudo snap install $SNAP_FILE |
niemeyer
reviewed
Jun 22, 2016
| + SNAP_FILE: "./$[SNAP_NAME]_1.0_all.snap" | ||
| + PLUG: network | ||
| + PORT: 8081 | ||
| + REQUEST_FILE: "./request.txt" |
niemeyer
reviewed
Jun 22, 2016
| + EOF | ||
| + | ||
| +restore: | | ||
| + sudo snap remove $SNAP_NAME |
niemeyer
Jun 22, 2016
Contributor
This must not be required. We're resetting the state on the suite.
niemeyer
reviewed
Jun 22, 2016
| + while ! netstat -lnt | grep -Pq "tcp.*?:$PORT +.*?LISTEN\n*"; do sleep 0.5; done | ||
| + | ||
| + echo "And we store a basic HTTP request" | ||
| + cat > $REQUEST_FILE <<EOF |
niemeyer
Jun 22, 2016
Contributor
echo "GET / HTTP/1.0\n\n" > $REQUEST_FILE
But I believe REQUEST_FILE isn't being used at all?
niemeyer
reviewed
Jun 22, 2016
| + PLUG: network | ||
| + PORT: 8081 | ||
| + REQUEST_FILE: "./request.txt" | ||
| + SERVICE_FILE: "./service.sh" |
niemeyer
reviewed
Jun 22, 2016
| +restore: | | ||
| + sudo snap remove $SNAP_NAME | ||
| + rm -f $SNAP_FILE $REQUEST_FILE | ||
| + sudo systemctl stop $SERVICE_NAME |
niemeyer
Jun 22, 2016
•
Contributor
It's a good practice to first stop the activity, then remove content. Not sure if it makes a difference on this specific test, though.
fgimenez
Jun 22, 2016
Contributor
yes, the service only depends on SERVICE_FILE, and it wasn't being removed :) Now it's in the right order, the service is stopped and the file removed afterwards
fgimenez commentedJun 21, 2016
No description provided.