rustlings verify accepts option1.rs prematurely

[marienz]

For rustlings verify, mode "compile" really only means "compile": it does not run the resulting program. This means option1.rs (which has a main() that panics) passes before I've completed the exercise. I almost missed the entire exercise, as I was working through them based on the errors from rustlings verify.

If this behavior from rustlings verify is intentional, option1.rs needs to be changed to a "test" exercise (with either main() itself as test function or a trivial test function that calls main()).

Alternatively, rustlings verify could start running the code too. Changing it to use compile_and_run works but makes the output a little noisy (did confirm I didn't miss any other tests before option1.rs), so it might need a new or refactored function that only prints the output on failure.

Alternatively alternatively, rustlings could grow an "exec" mode for exercises like this one.

[cjpearce]

I totally agree that there needs to be a better way of going through these sorts of exercises. It feels like a lot of the exercises are meant for exploring how the language works, and it even appears that the intention is for them to be run multiple times.

As an example primitive_types3.rs gives the user feedback using print statements, which users will never see if they use verify like the intro message suggests.

let a = ???

if a.len() >= 100 {
    println!("Wow, that's a big array!");
} else {
    println!("Meh, I eat arrays like that for breakfast.");
}

It seems like it might be quite difficult to get verify working in a way that runs commands for the user without becoming really messy like you mentioned, but I could imagine the rustlings watch command printing the output of the last exercise to compile and then waiting for user input before moving to the next exercise.

It might make sense at that point to point people towards the rustlings watch command in the intro message, and maybe even give it a different name to make it feel slightly more accessable.

I'd happily put something like that together if it sounded like a reasonable solution. Other people have been confused by the way the exercises are used as well (#127).

[marienz]

I hadn't tried rustlings watch because I obsessively save my files all the time and figured triggering watch on each save would be distracting (and demotivating because I'd get lots of errors for partially completed examples), and also because it wasn't clear to me rustlings watch actually goes through the exercises in order. I thought rustlings watch wouldn't do anything until I edited something after starting it, so verify seemed more convenient.

So yeah, maybe the only problem here is that the homepage recommends verify (and mentions watch as an alternative "If you don't want to rerun verify every time you change a file").

There were indeed a few other tests that print interesting things (I think threads1.rs is another good example). But option1.rs was the only one I noticed that passes prematurely. For the other ones, they still initially fail under rustlings verify, and it's obvious when editing the exercise that I might want to run them separately to see what they print.

[komaeda]

I'm definitely up for rethinking the user experience flow behind watch, since it was created as a convenience method for verify. If anyone has any ideas and would like to invest time into writing up something like a proposal, that would be much appreciated!

[josephrocca]

I'm not sure I followed the above comments (I'm using watch rather than verify), so just in case this is anything new: Rustlings skips over error_handling/option1.rs when using rustlings watch:

...
✓ Successfully tested exercises/error_handling/errorsn.rs!
✓ Successfully compiled exercises/error_handling/option1.rs!
! Testing of exercises/error_handling/result1.rs failed! Please try again. Here's the output:
...

In any case, thanks to komaeda and contributors for this! It's a great little set of exercises.

[nkanderson]

I also ran into this issue using watch. I've opened PR #198 as a possible fix for the smaller issue of the option1.rs test passing prematurely, following @marienz's suggestion of creating a trivial test function. This wouldn't preclude the possibility of adding alternate output options for verify or adding an exec mode, as suggested above.

[Zazcallabah]

assert!(pop_too_much(), true);

I'm going through these exercises, and this confuses me. assert! takes only one argument right? Or one argument and a message, but true is a bad message, isn't it?

should it be assert_eq!(pop_too_much(), true);? but that also makes no sense, since the values being compared are booleans. So should it be assert!(pop_too_much());? What am I missing here?

[nkanderson]

Hey @Zazcallabah,

The point of the exercise is to guide the user to implement error handling for receiving a None value from pop such that the pop_too_much function does not panic. Once error handling is implemented, the pop_too_much function should reach the line where it returns true, rather than panicking.

I think your last suggestion of assert!(pop_too_much()); makes the most sense to me. I admit I missed using the most sensible assert option when creating the PR above.

With that change, would the goal of the exercise be clearer to you? Were you still seeing the exercise pass prematurely? If you think other changes might be necessary and you're not seeing the test pass prematurely, I'm wondering if it would be best to open a new issue or PR a change you think would clarify the exercise.

[Zazcallabah]

The exercise never passed prematurely - it worked fine. It was just when I read that specific assert I got confused if there was something I didn't understand or if there was an accidental extra parameter in the assert.

Thank you for clarifying. :D

[nkanderson]

No problem, @Zazcallabah, and good call on cleaning up the assert!

@marienz (or a project maintainer?), do you think it makes sense to update the title of this issue, since the specific bug of the test passing prematurely has been addressed? I think the conversation about altering verify and watch seems worth continuing (or at least it was still relevant when I was working through these exercises a few months ago), but that's a broader cli behavior change than the bug fix implied by the title.

[shadows-withal]

Gonna close this, feel free to open a separate issue if you want to continue talking about altering verify or watch!