improve test scripts #44

zimbatm · 2017-12-25T18:00:44Z

validated with shellcheck
allow to evaluate just one for convenience
reduce verbosity and improve debugging output

* validated with shellcheck * allow to evaluate just one for convenience * reduce verbosity and improve debugging output

lukateras

@zimbatm, I really appreciate that you made Travis CI to work, and I do appreciate the time you've spent on doing this, but...

This turns a 15-line shell script into 45 lines without much return.

It hides useful information (list of derivations that will be built might be useful, e.g. it might help debug stuck builds). CI logs should be as verbose as possible. When my local checks stutter with download-from-binary-cache.pl, I also want to know that.

It introduces a checker that has to be babysitted by adding magical comments and makes the script much harder to change without causing subsequent checks to fail (see NixOS/nixpkgs#32857).

It also includes redundant formatting that mostly duplicates what is currently being emitted by the script or by nix-build command.

I think this is over-engineered out of proportion and what we currently have (roughly, your second rebase of #40) is close to being ideal, other than missing --show-trace.

lukateras · 2017-12-25T18:13:06Z

tests/eval-one.sh

@@ -0,0 +1,35 @@
+#!/bin/sh


Evaluating just one is not a proper test, because profiles can be inherited (even model profiles, because there might be revisions, e.g. later revisions of X220 do not support tp_smapi).

It can be done via nixos-rebuild -I nixos-config=tests/eval-test.nix nixos-hardware-config=path/to/default.nix dry-build. I think it's a bad idea to allow to eval just one profile via a script.

Travis will evaluate all of them in the end, or not?

Travis evaluates every profile on each push or pull request.

The idea here is that I can test my profile without having to wait on all the others to evaluate. Travis is testing all the profiles anyways.

You shouldn't assume that your profile isn't inherited by any other profiles. Profile might be inherited by someone from another directory, e.g. in case of brand renaming (e.g. IBM ThinkPad -> Lenovo ThinkPad), including a model profile.

How is this relevant? Travis will catch this eventually.

It's relevant in that it's not a test and thus should not be exposed. It's possible to see someone with merge rights using eval-one test for convenience and then pushing to master.

Actually, doing that is the only use case I can come up with in regards to this script.

lukateras · 2017-12-25T18:13:54Z

tests/eval-test.sh

@@ -1,15 +1,11 @@
 #!/bin/sh
+set -efu
+cd "$(dirname "$0")/.." || exit 1


This can't really fail.

|| exit 1 should not be necessary, since set -e is used.

lukateras · 2017-12-25T18:17:06Z

tests/eval-one.sh

+ret=$?
+
+if [ "$ret" -gt 0 ]; then
+  echo ERROR


nix-build already prints "error", we don't need to print that twice.

Look at the Travis CI output logs and you will understand.

I don't understand: https://travis-ci.org/NixOS/nixos-hardware/builds/321451543#L5488

lukateras · 2017-12-25T18:17:36Z

tests/eval-one.sh

+  -I "nixos-config=$here/eval-test.nix" \
+  -I "nixos-hardware-profile=$profile" \
+  --dry-run \
+  --no-out-link \


--no-out-link doesn't do anything when --dry-run is present.

lukateras · 2017-12-25T18:18:33Z

tests/eval-one.sh

+nix-build \
+  '<nixpkgs/nixos>' \
+  -I "nixos-config=$here/eval-test.nix" \
+  -I "nixos-hardware-profile=$profile" \


We shouldn't allow profiles that have spaces in names.

It doesn't hurt to escape the argument in either case

It does. We should enforce that there are no spaces in names.

lukateras · 2017-12-25T18:19:46Z

tests/eval-one.sh

+  --dry-run \
+  --no-out-link \
+  --show-trace \
+  -A system > "$logfile" 2>&1


I don't see the point of having a log file instead of stderr, and I think it hides a lot of potentially useful information. Also, the idea of stderr is exactly to have an error log.

The test will only produce output when the test fail and will output both stdout and stderr. This sounds good to me.

It doesn't matter, I think nix-build only ever writes to stderr. What matters is that sometimes CI builds may stutter, and with this approach, we'll never know why.

Also 2>&1 is a bashism.

@yegortimoshenko With this approach it's easy to find the failing profile and get to the point instead of going through pages of repeating out links. They might be interesting but not very useful in practice.

Right, you only need logs if something goes wrong. Your way of doing things makes certain errors opaque and non-debuggable.

lukateras · 2017-12-25T18:20:06Z

tests/eval-test.sh

-set -e
-
-cd "$(dirname "$0")/.."
+echo "### Evaluating all profiles ###"


This is redundant.

lukateras · 2017-12-25T18:22:39Z

tests/eval-one.sh

+profile=$1
+logfile=$(mktemp)
+
+# shellcheck disable=SC2039


What's the point of using a tool when it requires magic comments to do its job? I think shellcheck is very overreaching, and we shouldn't use it.

It catches a lot of common mistakes and educates the users about shell corner-cases.

We don't need that for fifteen lines of shell script. And teaching people to use something doesn't make magical comments OK.

lukateras · 2017-12-25T18:48:29Z

tests/eval-one.sh

+  -I "nixos-hardware-profile=$profile" \
+  --dry-run \
+  --no-out-link \
+  --show-trace \


--show-trace might be useful and should make it into master.

lukateras · 2017-12-25T19:07:10Z

tests/eval-test.sh


+# shellcheck disable=SC2044


Not sure why you ignore this check: https://github.com/koalaman/shellcheck/wiki/SC2044

In this case, it could be rewritten as find . -name default.nix -exec tests/eval-one.sh {}. But again, I think we shouldn't accept files with weird names into this repo, and current shell script makes sure that we won't.

There is no solution that doesn't depend on word splitting, without depending on bash 4. find -exec doesn't early-return nor propagate the exit status. I don't think that profiles should contain special characters in any case.

Mic92 · 2017-12-25T22:27:17Z

tests/eval-test.sh

@@ -1,15 +1,11 @@
 #!/bin/sh
+set -efu


set -o pipefail also comes handy.

It is a bashism.

lukateras · 2018-07-17T13:53:00Z

Resolved in #66 with a fundamentally different approach.

lukateras · 2018-11-02T17:33:28Z

@zimbatm I shouldn't have behaved so aggressively over this. I'm sorry.

improve test scripts

8229a43

* validated with shellcheck * allow to evaluate just one for convenience * reduce verbosity and improve debugging output

zimbatm mentioned this pull request Dec 25, 2017

add Travis-CI to the repo #40

Merged

2 tasks

lukateras requested changes Dec 25, 2017

View reviewed changes

lukateras reviewed Dec 25, 2017

View reviewed changes

lukateras added a commit that referenced this pull request Dec 25, 2017

tests/eval-test: Add --show-trace from #44

f1952ea

Mic92 reviewed Dec 25, 2017

View reviewed changes

fixup! improve test scripts

0aa778c

lukateras closed this Jul 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve test scripts #44

improve test scripts #44

zimbatm commented Dec 25, 2017

lukateras left a comment •

edited

lukateras Dec 25, 2017 •

edited

Mic92 Dec 25, 2017

lukateras Dec 25, 2017 •

edited

zimbatm Dec 26, 2017

lukateras Dec 26, 2017 •

edited

Mic92 Dec 26, 2017

lukateras Dec 26, 2017 •

edited

lukateras Dec 25, 2017

Mic92 Dec 25, 2017 •

edited

lukateras Dec 25, 2017 •

edited

zimbatm Dec 26, 2017

lukateras Dec 26, 2017

lukateras Dec 25, 2017

lukateras Dec 25, 2017

zimbatm Dec 26, 2017

lukateras Dec 26, 2017 •

edited

lukateras Dec 25, 2017

Mic92 Dec 25, 2017 •

edited

lukateras Dec 25, 2017 •

edited

lukateras Dec 25, 2017

zimbatm Dec 26, 2017

lukateras Dec 26, 2017

lukateras Dec 25, 2017

lukateras Dec 25, 2017

zimbatm Dec 26, 2017

lukateras Dec 26, 2017 •

edited

lukateras Dec 25, 2017

lukateras Dec 25, 2017

zimbatm Dec 26, 2017

Mic92 Dec 25, 2017

lukateras Dec 25, 2017

lukateras commented Jul 17, 2018

lukateras commented Nov 2, 2018 •

edited

improve test scripts #44

improve test scripts #44

Conversation

zimbatm commented Dec 25, 2017

lukateras left a comment • edited

Choose a reason for hiding this comment

lukateras Dec 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 25, 2017 • edited

Choose a reason for hiding this comment

lukateras Dec 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Dec 25, 2017 • edited

Choose a reason for hiding this comment

lukateras Dec 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras Dec 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukateras commented Jul 17, 2018

lukateras commented Nov 2, 2018 • edited

lukateras left a comment •

edited

lukateras Dec 25, 2017 •

edited

lukateras Dec 25, 2017 •

edited

lukateras Dec 26, 2017 •

edited

lukateras Dec 26, 2017 •

edited

Mic92 Dec 25, 2017 •

edited

lukateras Dec 25, 2017 •

edited

lukateras Dec 26, 2017 •

edited

Mic92 Dec 25, 2017 •

edited

lukateras Dec 25, 2017 •

edited

lukateras Dec 26, 2017 •

edited

lukateras commented Nov 2, 2018 •

edited