-
Notifications
You must be signed in to change notification settings - Fork 71
Add a regression test for `learn-ocaml build' #353
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
erikmd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @agrn ;
LGTM, one remark though: there are several occurrences of the pattern
for CORPUS in $(find ./corpuses -mindepth 1 -maxdepth 1 -type d) ; do …; done
but this won't work if ever there is a folder that contains a space (I know this is a priori not the case, but better safe than sorry)
so as suggested in https://mywiki.wooledge.org/BashFAQ/024#Workarounds you may want to phrase your loops like this:
while IFS= read -r CORPUS; do …; done < <(find ./corpuses -mindepth 1 -maxdepth 1 -type d)
BTW feel free to write your bash variable names as lowercase: corpus i/o CORPUS, as by convention, lowercase variables are not intended to be exported as environment variables, and won't thereby clash with (uppercase) environment variables :)
|
Thank you for your feedback @erikmd!
LGTM, one remark though: there are several occurrences of the pattern
```
for CORPUS in $(find ./corpuses -mindepth 1 -maxdepth 1 -type d) ; do …; done
```
but this won't work if ever there is a folder that contains a space (I know this is a priori not the case, but better safe than sorry)
so as suggested in https://mywiki.wooledge.org/BashFAQ/024#Workarounds you may want to phrase your loops like this:
```
while IFS= read -r CORPUS; do …; done < <(find ./corpuses -mindepth 1 -maxdepth 1 -type d)
```
Eww, as if I needed to be reminded why I hate shell scripts with a
burning passion. :)
I am fixing this as I am writing these lines.
|
tests/runtests.sh
Outdated
| pushd "$dir" > /dev/null | ||
|
|
||
| echo "---> Doing $DIR:" | ||
| echo "---> Doing $dir:" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| echo "---> Doing $dir:" | |
| echo "---> Entering $dir:" |
|
This looks great to me. Did you try to run shellcheck on your script? |
|
Yes, it reports the usual errors with |
This splits the big for-loop into subroutines to improve the readability of the script. Variables are also quotted to avoid issues. Most variables are also renamed to be in lowercase, and for loops replaced by while loops to be more reliable. Signed-off-by: Alban Gruin <[email protected]>
To check for regressions for the issue ocaml-sf#305 fixed by 5a661e4 ("repo: fix repo generation when an exercise has multiple subdirectories", 2020-05-26), this adds an optional test that builds repositories found in tests/corpuses/. No such repository are provided as-is here, but a CI pipeline may want to clone a corpus (eg. `learn-ocaml-corpus') in this directory. Signed-off-by: Alban Gruin <[email protected]>
…n-ocaml-corpus The previous commit added a mechanism to try to build repositories found in tests/corpuses/. This adds a directive in .travis.yml to clone learn-ocaml-corpus, so `lean-ocaml build' can be tested by the CI. Signed-off-by: Alban Gruin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
there is also the only for loop that I did not convert
Granted, but fortunately the loop at stake only deal with *.ml files which (by definition) contain no space:
Line 109 in de6191e
| for tosend in $(find . -name "*.ml" -type f -printf "%f\n") |
So keeping the PR as is is very OK I guess.
|
Thanks! |
Commit 5a661e4 ("repo: fix repo generation when an exercise has multiple subdirectories", 2020-05-26) fixed the issue #305, but it did not add any regression test to the test suite to ensure that the behaviour of `learn-ocaml build' stayed correct.
This series cleans up a bit
runtests.sh, adds such a test, and makes Travis to check `learn-ocaml build' against learn-ocaml-corpus.This patch series keeps the general behaviour of
runtests.sh-- perhaps in the future, it could be changed to have a TAP-compliant output, or allow it to run without docker?