Add a utility to assist manually testing an exercise #381

coriolinus · 2017-11-02T23:34:01Z

Instead of having to manually enable tests, copy the examples, and
clean up, this script automates all that.

petertseng

What I think should happen next is that https://github.com/exercism/rust/blob/master/_test/check-exercises.sh should simply run this script. That way there is less duplicate code and we know that what we use to test locally is the same as what Travis uses.

petertseng · 2017-11-03T01:03:37Z

bin/test-exercise

+# run tests
+cargo test
+
+# reset


optionally, all this reset business may move into a trap so that it will be run even if the script gets Ctrl-C in the middle of the cargo test run

I don't require it because perhaps that will be a rare occurrence.

One can see an example at https://github.com/exercism/go/blob/master/bin/test-without-stubs#L24-L39

coriolinus · 2017-11-03T01:54:59Z

Agree with both comments. I'll update in the morning.

coriolinus · 2017-11-03T20:21:45Z

Delaying this because today was crazy. Will come back to it later. Integrating it into check-exercises.sh properly is a bigger task than it looked at first.

petertseng · 2017-11-03T22:13:12Z

Yes, it seems to be bigger because check-exercises operates in separate modes: run the tests, compile the tests with warnings, and run benchmarks. Will have to figure that out

coriolinus · 2017-11-07T10:28:50Z

That's interesting:

$ ./_test/check-exercises.sh
./_test/check-exercises.sh: line 41: bin/test-exercise: Permission denied
The command "./_test/check-exercises.sh" exited with 1.

Not sure what that means; there are no crazy permissions on test-exercise. Looking into it.

coriolinus · 2017-11-07T10:59:11Z

Test fail, but it's not the fault of this PR:

$ ./_test/check-exercises.sh
The command "./_test/check-exercises.sh" exited with 0.
0.05s$ sh ./_test/ensure-lib-src-rs-exist.sh
The command "sh ./_test/ensure-lib-src-rs-exist.sh" exited with 0.
7.65s$ sh ./_test/ensure-stubs-compile.sh
error: unused variable: `puzzle`
 --> src/lib.rs:4:14
  |
4 | pub fn solve(puzzle: &str) -> Option<HashMap<char, u8>> {
  |              ^^^^^^
  |
note: lint level defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: #[deny(unused_variables)] implied by #[deny(warnings)]
  = note: to disable this warning, consider using `_puzzle` instead
error: aborting due to previous error
error: unused variable: `puzzle`
 --> src/lib.rs:4:14
  |
4 | pub fn solve(puzzle: &str) -> Option<HashMap<char, u8>> {
  |              ^^^^^^
  |
note: lint level defined here
 --> src/lib.rs:1:9
  |
1 | #![deny(warnings)]
  |         ^^^^^^^^
  = note: #[deny(unused_variables)] implied by #[deny(warnings)]
  = note: to disable this warning, consider using `_puzzle` instead
error: aborting due to previous error
To learn more, run the command again with --verbose.
alphametics's stub does not compile; please make it compile or remove all non-commented lines
decimal's stub is allowed to not compile
Exercises that don't compile:
alphametics
The command "sh ./_test/ensure-stubs-compile.sh" exited with 1.

We should update alphametics, but I believe that a different PR should handle that.

petertseng · 2017-11-07T23:53:26Z

_test/check-exercises.sh

+   directory=$(dirname "${exercise}");
+
+   if [ -n "$DENYWARNINGS" ]; then
+      sed -i -e '1i #![deny(warnings)]' "$directory/src/lib.rs"


This places a [deny(warnings)] line in each stub file, which promptly gets replaced by each example file. Therefore, no [deny(warnings)] line is present in any file that gets compiled by cargo.

The original script placed a [deny(warnings)] in each example file instead, and I believe that being in the example file is the desired behaviour.

Good catch; I'll need to change that.

petertseng · 2017-11-07T23:55:41Z

_test/check-exercises.sh

 else
    files=exercises/*/tests
+    release="--release"


I proposed this change in #241 and it was rejected, for the reason:

A reason to not use release mode: it will skip overflow checks

I support this change. Please be aware of the reasons for the previous rejection.

I put that in to support testing of the script locally, but I'm only moderately in favor of it. It makes sense to ensure that nothing overflows in the tests, so I'll need to get rid of that.

A possible nice change to make would to test in release mode if .meta/test-in-release-mode is present, or something along those lines.

petertseng · 2017-11-07T23:58:42Z

_test/check-exercises.sh

+      ./bin/test-exercise $directory $release
+   fi
+   # as the test-exercise command is the last one run in all cases,
+   # its exit code is preserved automatically


true statement, but we do not have -e in DENYWARNINGS mode, therefore the exit code is the exit code of that of the last exercise, is it not? Thus, we would only get a nonzero exit if the very last exercise has any warnings, and all other exercises' warnings are ignored, I think?

I see. Just tested, and you're correct. I'll update that.

coriolinus · 2017-11-08T00:58:07Z

Good idea; that will be quick to add.

…

On Wed, Nov 8, 2017 at 1:56 AM, Peter Tseng ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In _test/check-exercises.sh <#381 (comment)>: > else files=exercises/*/tests + release="--release" A possible nice change to make would to test in release mode if .meta/test-in-release-mode is present, or something along those lines. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#381 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHdeTmXg71OhlY7v1BVoiSVA7XszEjJyks5s0PxYgaJpZM4QQfPt> .

petertseng · 2017-11-08T00:58:39Z

_test/check-exercises.sh

 if [ -n "$BENCHMARK" ]; then
    files=exercises/*/benches
+    # cargo bench --release is an invalid command


this comment is no longer makes sense since --release is not being added here

petertseng

Conditional on removing that comment that no longer makes sense in #381 (comment).

I tested this by running it locally on one exercise, but I did not test:

other exercises
BENCHMARK
DENYWARNINGS

because my allotted time has elapsed therefore I do not have the time to test these things thoroughly.

The comments associated with this review are comments on existing code that got moved by this PR, not new code or change in behaviour.

petertseng · 2017-11-08T01:06:38Z

_test/check-exercises.sh

-    fi
+   # This assumes that exercises are only one directory deep
+   # and that the primary module is named the same as the directory
+   directory=$(dirname "${exercise}");


this line is not yours (you only unindented it) so I think the change should come later, but I note that the semicolon is unnecessary.

petertseng · 2017-11-08T01:08:21Z

bin/test-exercise

+
+# eliminate #[ignore] lines from tests
+for test in "$exercise/tests/*.rs"; do
+    sed -i '/\[ignore\]/d' $test


this line is also not yours (you only moved it from one file to another), so shouldn't get fixed here, but it's worth noting that on BSD seds, this gets a:

sed: 1: "exercises/accumulate/te ...": invalid command code e

Whereas it would work if a -e were added, or the -i were changed to -i ''.

petertseng

I think in my ideal world I would squash in such a way that the final product contains two commits:

One that moves existing functionality into bin/test-exercise, at which point all observable behaviour is expected to remain the same.
One that adds the .meta/test-in-release-mode file and associated code and functionality.

If a different sequence of commits seems to make sense, say the word.

coriolinus · 2017-11-08T08:21:04Z

Ok, I think that squash takes us most of the way there. There's one deviation from the desired end state: removing the extraneous comment is in the second commit instead of the first, because git rebase -i can squash commits but cannot move changes between commits.

I'm going to add one more commit to this, to take care of the nitpicks regarding lines copied from the previous implementation, and then I think this will be ready to merge.

petertseng

I agree with what you say.

were it up to me, I would rebase -i again and edit the first commit deleting the comment lines:

    # cargo bench --release is an invalid command

and

   # as the test-exercise command is the last one run in all cases,
   # its exit code is preserved automatically

I think that would be the most ideal, to make commits contain exactly what they say and no more.

I suppose if asked the question I wouldn't mind taking it as is, but since we are close to what seems like an ideal, might as well go all the way there, I feel?

Instead of having to manually enable tests, copy the examples, and clean up, this script automates all that. - Take optional exercise path argument - Express all paths in terms of exercise path - Pass further arguments through to cargo - Use arrays to simplify file/dir preservation and reset - Use trap to ensure reset occurs on ctrl-c - Express check-exercises.sh in terms of bin/test-exercise - Add debug tests to check-exercises in case of remote execution failure - Move $![deny(warnings)] insertion into test-exercise. - Give the feature to people testing the exercise on its own - Clean up properly after - Return the bitwise OR of individual exercises' return values That last one wants some commenting. If a bunch of values fail in different ways, this is of limited utility; your return code won't be 0, and Travis will fail, but you won't get the info you expected from the return codes. However, this does have some useful properties: - if a bunch of things fail in the same way, or only one thing fails, you get Cargo's return code back directly - you discard less overall information than the naïve implementation ( if [ $? != 0 ]; then return_code=1; fi ) - always returns zero or non-zero appropriately without branching

- Compile in release mode if .meta/test-in-release-mode exists - Add note to README documenting the above feature - Add .meta/test-in-release-mode to alphametics, which is the sore spot for debug-mode tests

coriolinus · 2017-11-08T13:22:42Z

We've gone all the way there! I'll merge as soon as this passes Travis.

petertseng approved these changes Nov 3, 2017

View reviewed changes

coriolinus mentioned this pull request Nov 7, 2017

Remove unused variable from Alphametics stub #388

Merged

petertseng suggested changes Nov 7, 2017

View reviewed changes

petertseng reviewed Nov 7, 2017

View reviewed changes

petertseng suggested changes Nov 7, 2017

View reviewed changes

coriolinus force-pushed the add-test-exercise-script branch from 35d473d to c9c1e67 Compare November 8, 2017 00:55

petertseng reviewed Nov 8, 2017

View reviewed changes

petertseng approved these changes Nov 8, 2017

View reviewed changes

petertseng reviewed Nov 8, 2017

View reviewed changes

coriolinus force-pushed the add-test-exercise-script branch from 835e1e1 to acee1d7 Compare November 8, 2017 08:15

petertseng approved these changes Nov 8, 2017

View reviewed changes

coriolinus added 3 commits November 8, 2017 14:19

Add feature: compile in release mode for exercises with a .meta flag

9c4221c

- Compile in release mode if .meta/test-in-release-mode exists - Add note to README documenting the above feature - Add .meta/test-in-release-mode to alphametics, which is the sore spot for debug-mode tests

Remove extraneous ';' from script

0691ef3

coriolinus force-pushed the add-test-exercise-script branch from 6fdc183 to 0691ef3 Compare November 8, 2017 13:21

coriolinus merged commit 44be03a into exercism:master Nov 8, 2017

coriolinus deleted the add-test-exercise-script branch November 8, 2017 13:32

This was referenced Mar 20, 2018

macros: Allow stub to not compile #482

Merged

ensure-stubs-compile: Ensure compilation: Touch src/lib.rs #483

Merged

Add a utility to assist manually testing an exercise #381

Add a utility to assist manually testing an exercise #381

Uh oh!

Conversation

coriolinus commented Nov 2, 2017

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 3, 2017

Uh oh!

coriolinus commented Nov 3, 2017

Uh oh!

petertseng commented Nov 3, 2017

Uh oh!

coriolinus commented Nov 7, 2017

Uh oh!

coriolinus commented Nov 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 8, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 8, 2017

Uh oh!

petertseng left a comment

Choose a reason for hiding this comment

Uh oh!

coriolinus commented Nov 8, 2017

Uh oh!

Uh oh!

petertseng left a comment •

edited

Loading