More tests

[jankatins]

No description provided.

[flying-sheep]

this still contains the commits from #324…

maybe you need to rebase this on master?

[jankatins]

I'm currently just trying if/how that works (currently it doesn't...)...

[flying-sheep]

maybe:

[[ "$TRAVIS_OS_NAME" == "osx" ]] && os=MacOSX || os=Linux
wget "https://repo.continuum.io/miniconda/Miniconda3-latest-$os-x86_64.sh" -O miniconda.sh

[jankatins]

nice

[jankatins]

So I think we run her into the same problems as all Mac OSx users:

0.00s$ cat "$INSTALL_LOG"
* installing *source* package ‘IRkernel’ ...
** R
** inst
** preparing package for lazy loading
Error : .onLoad failed in loadNamespace() for 'pbdZMQ', details:
  call: dyn.load(file, DLLpath = DLLpath, ...)
  error: unable to load shared object '/Users/travis/R/Library/pbdZMQ/libs/pbdZMQ.so':
  dlopen(/Users/travis/R/Library/pbdZMQ/libs/pbdZMQ.so, 6): Library not loaded: /Builds/CRAN-QA-Simon/packages/mavericks-x86_64/Rlib/3.3/pbdZMQ/libs/libzmq.4.dylib
  Referenced from: /Users/travis/R/Library/pbdZMQ/libs/pbdZMQ.so
  Reason: image not found
ERROR: lazy loading failed for package ‘IRkernel’
* removing ‘/Users/travis/build/IRkernel/IRkernel/IRkernel.Rcheck/IRkernel’

https://travis-ci.org/IRkernel/IRkernel/jobs/130009741

Do you think it makes sense to ask the pbdZMQ package to statically link to the underlying lib?

[jankatins]

So, and then there is wondows: https://github.com/craigcitro/r-travis

[flying-sheep]

eh?

NATIVE R SUPPORT IN TRAVIS IS LIVE!

we already use that…

[jankatins]

Damn, wrong Link, have to find the right one again...

[flying-sheep]

this already exists when running on travis. please remove it.

[jankatins]

yep, that was a problem when I executed instead of sourced the file

[flying-sheep]

OK, then please remove the function and go back to sourcing it.

[jankatins]

AFAIR the reason for not sourcing was a problem with set -e which terminated the whole build including the after_failure clause in the travis script. Running it as bash script and setting set -e fixed that.

Without set -e you have to check each command so that the build fails if one of the steps fails (e.g. the problem you noticed that the test builds do not show that something is wrong...).

I will try to get a fix for both problems... Going back to a "travis.yml only" setup would be my preference... IMO one big travis.yml is better than a travis.yml which links to a big script. Especially if the amount of code is reduced because we don't need to reimplement most of travis' r support?

[jankatins]

See also here: https://docs.travis-ci.com/user/customizing-the-build/#Implementing-Complex-Build-Steps -> calling the script with set -ev

[flying-sheep]

Going back to a "travis.yml only" setup would be my preference...

i tried it, but i didn’t find a way to get folding then. separate shell lines in YAML are treated as separate commands, which messes with the folding, …

maybe we can put it all in the YAML like this?

script:
  - |
    travis_fold start Building
    bootstrap_header 'Building package'
    R CMD build .
    travis_fold end Building
  - ...

[jankatins]

That looks like a great idea! Will try that, but it could be a few days until I can come back to this... I also would like to have a solution for the zmw problem on OSX first...

[jankatins]

This is usefull: https://lint.travis-ci.org/ ...

[flying-sheep]

you think so?

in language section: illegal value r, defaulting to ruby

this is obviously wrong. travis has first-class R support.

[jankatins]

R is not officially supported... I had more problems which ended up not even starting a travis run... for that it was great :-)

[flying-sheep]

ah OK. btw.: don’t bother potting it all back into .travis.yml if you don’t manage to get the folding to work. better check this first before you invest too much time.

[jankatins]

Folding is coming back...

[flying-sheep]

great! will it also fail properly or does only the return value of the last command (which would be the folding) count?

[jankatins]

I've also move the install steps back as my understanding of set -e and bash functions is currently, that they don't affect it: a error in one of the steps would not jump out of the function and therefore would not cancel the build.

test.sh:

fold_wrap() {
    local header=$1; shift
    local section=$1; shift
    local cmd=$@
    echo start $section $header
    "$cmd"
    local result=$?
    echo end $section
    return $result
}

example_command() {
    set -e
    echo Hallo
    false
    echo SHOULD NOT HAPPEN
    set +e
}

$ source test.sh
$ fold_wrap HEAD SEC example_command || echo "BAD"
start SEC HEAD
Hallo
SHOULD NOT HAPPEN
end SEC

[flying-sheep]

the problem isn’t (only) scrolling, it’s the obscurity of some of the lines. a folding section that says “testing R CMD check logs for warnings and errors” before something like grep -q 'Status..1 NOTE' "$CHECK_LOG" is valuable.

but yeah: i veto the huge block of obscure commands with no wrapping and helpful info. i’m sure we’ll find a solution though.

how about we use || exit 1 in a function for the two-liners and bash scripts for the longer things?

[jankatins]

As I said: I find it fine as is. I've searched through matplotlibs errors and that is a lot worse.

Ask yourself: how much time do you spend scrolling vs the time you spend adding that folding (and breaking the tests for several days/weeks until we noticed)...

Simple travis.yml are best and we are already way past simple...

[takluyver]

I'm inclined to side with simplicity here over the presentation of test runs. Folding sections of output is nice if it's convenient, but let's not spend too much time on it if that's the only obstacle. We can always come back to it later if Travis changes something or we think of a neater way to do it.

[flying-sheep]

it’s easy, it just requires to split it up into files which @JanSchulz doesn’t like to do.

[takluyver]

Split tests up, or split test infrastructure up? I also would rather not have a bunch of files just for running Travis.

[flying-sheep]

me neither but it’s better than having the build terminate or a messy output.

@JanSchulz i’ll also do it myself if you don’t want to, i just want to make sure we’re all on the same page

[takluyver]

Doing it like this with a separate class for each test means that it re-discovers and then skips the base jupyter_kernel_test tests for each subclass. Is that intentional?

[jankatins]

My intention was that the kernel was restarted for each test... as I play with options in the tests at the end, it seems to do that.

[takluyver]

On 23 May 2016 at 16:43, Jan Schulz notifications@github.com wrote:

My intention was that the kernel was restarted for each test... as I play
with options it seems to do that.

OK. By default JKT reuses a kernel between tests, because starting it up
and spinning it down can be slow, and the tests are written not too depend
on order. If you want to create a new kernel for each test, I'd recommend
using setUp and tearDown methods - essentially equivalent to the setUpClass
and tearDownClass methods that are already there:
https://github.com/jupyter/jupyter_kernel_test/blob/6c392ddf54a24c464fa5e07ad7fb001023f04698/jupyter_kernel_test/__init__.py#L21

[jankatins]

Ok, I reordered... somehow the kernel tester looks like it would be a good fit for pytest fixtures :-)

[takluyver]

I prefer to keep JKT at the simple common layer of unittest, but you've seen how little code it is to start a kernel, so if you want to make a pytest fixture for it, I'm fine with that. We could add it to JKT as a module.

[takluyver]

Let's merge this with the Travis infrastructure as @JanSchulz has done it, and then you can do a PR to make the output how you want, and we can weigh up the added complexity versus the messiness of the output.

[flying-sheep]

actually i’m happy with the current tradeoff compared to the changed version.

so i’d say we leave the terminating build/good output combo as it is (only merge the other changes now), or settle on a solution before we merge this, then only discuss details (but nothing fundamental) in my PR.

my proposal: everything above 2-3 trivial lines goes into .travis/name_of_step.sh, in which set -e is used. the trivial steps go into functions in which the individual functions are guarded via || exit 1. scripts and functions are called using my wrapper above. are you both OK with this?

[jankatins]

everything above 2-3 trivial lines goes into .travis/name_of_step.sh, in which set -e is used. the trivial steps go into functions in which the individual functions are guarded via || exit 1. scripts and functions are called using my wrapper above. are you both OK with this?

No: simple travis >> good-looking test logs

The beautiful version had bugs, adding each test directly in travis.yml prevents that

[flying-sheep]

“No” won’t do 😆

This needs to be maintainable and absolutely isn’t.

do you have a better idea than me how to organize this? i’m seriously not willing to merge anything as underdocumented and unclear as this. (it’s great work otherwise of course! it just needs to be clearly structured)

i have to spend a few seconds on each line to find out what it does and to mentally reconstruct the rationale why it is there. slapping on comments on each individual command won’t improve this much, as it’ll lead to an even bigger, noisier block of code.

if you want this merged you’ll need to agree to some solution that structures the commands into a small number of steps that can be folded and have a good description. as said: you don’t have to do it, i will, but you’ll have to agree on it beforehand.

---

another idea:

define functions in .travis.yml with || exit 1 after each line and then call the wrapper on those functions?

⇒ we can’t leverage set -e but it’ll be one file.

[takluyver]

The before_install section looks reasonable to me, but the script section is a bit opaque. I don't know how much it's possible to simplify that if we want to test the output from R CMD check, though.

[flying-sheep]

HA YES!

script:
  - |
    wrap_cmd() {
      ...
    }
  - |
    some_step()
      trap 'trap - ERR; return' ERR
      do_stuff
      idk_lol
    }
  ...
  - wrap_cmd 'Some step' some_step
  - wrap_cmd ...
  - ...

[takluyver]

Hmm, bash functions using trap statements inside a YAML file is a long way from what I'd call elegant ;-)

[flying-sheep]

that’s only because bash isn’t elegant, and swallows errors by default, and because travis isn’t elegant, and uses bash for scripting. i’m all about that elegancy though, so if you have a better idea…

i’d prefer just sourcing one big bash file, this way we avoid subtle YAML encoding gotchas (if such a thing exists) and gain syntax highlighting at the cost of having one more file.

what do you both think?

[jankatins]

I want a single line per test in the travis.yaml and certainly not a wrapper. This has the disadvantage that the log will be slightly longer and more verbose (but it's still a long way from what I know in other projects), but it is a lot less prone to failure (=current situation). And it doesn't waste developer time when this has to be implemented (e.g. I think the time we both spent on this issue arguing is more that we both will ever spent on scrolling through the log).

IMO this PR is "ready to be merged" as it is.

I'm not going to implement a wrapping style, feel free to do it yourself, but also make sure that it fails when one of the steps go wrong, so that we don't end up in the current situation where we basically had no tests.

[flying-sheep]

there’s also an alternative which is more clear!

some_step() (
    set -e
    im_in_a_subshell
)

[flying-sheep]

hey, sorry to be confrontative, but you really need to read my stuff more carefully.

i said multiple times that i only want you to agree on me changing stuff up afterwards, not implement it yourself.

now you propose the same thing. you could simply have said from the beginning that you’re OK with whatever i do as long as i don’t create multiple files.

so if you’re both OK with the following course of action, please say so and we can go ahead:

1. merge this
2. i move everything into a separate .sh file to gain syntax highlighting (optional, please state your opinions)
3. i move all sections into individual functions (with set -e in a subshell so the whole function fails if a command fails)
4. i call those functions with a wrapper that adds folding inside of .travis.yml

it would look like:

#.travis.sh
wrap_cmd() { ... }
step_a() (
    set -e
    ...
)
...

#.travis.yml
script:
    - source .travis.sh
    - wrap_cmd 'Step A' step_a
    - ...

OK? @JanSchulz @takluyver?

[takluyver]

That sounds fine to me. I mean, it sounds totally crazy that you can define functions with different kinds of brackets and they work in deeply different ways, but that's bash for you.

[jankatins]

As I said: My vote goes to keeping it as is in this PR without putting in a wrapper and voodoo magic just for the sake of a shorter log and a few less seconds per log read. But I also don't want to waste any more time on this than I already have.

If you want to spend the time:

make sure that it fails when one of the steps go wrong, so that we don't end up in the current situation where we basically had no tests.

Please also document the different kind of brackets and so on in all places, that no one is accidentally breaking it in a later change to the test system. I didn't even know that such stuff exists.

[takluyver]

Alright, thanks both.

[flying-sheep]

in bash, foo() compound_command defines a function. compound commands can be loops, conditionals, and groupings.

the two kinds of groupings are those: {} is a command list (same environment/shell) and () is a subshell (new, inheriting environment/shell). you can use both to e.g. redirect collected output into a single command:

{
    echo '<!doctype html><html>'
    produce_html
    echo '</html>'
} | expect_html_document

i use subshells often interactively for this:

(cd somewhere; command)

which will not change my directory if the command fails. (unlike pushd somewhere; command; popd)

bash is fucking complex. i also didn’t know that any kind of compound command is accepted as function body, but i think everyone should know about subshells.