Skip to content
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

feature: adds "output" mode, resolves #270 #278

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

jrvidal
Copy link
Contributor

@jrvidal jrvidal commented Feb 26, 2020

  • New: mode = "output.
  • If output = "foobar" is specified for an "output" exercise, it runs a "test" against the expected output.
  • An "output" exercise prints its output in the 🎉 Move on when you're ready screen.
Screenshots

Screenshot at 2020-02-26 21:52:49
Screenshot at 2020-02-26 21:52:23

After playing a while with this, I'm not entirely sold on the idea 😅 I think having expected output could be useful in a very exploratory exercise like "Get main to print Hello world! to the screen". But, in general, I feel like "test" exercises guide you better toward the answer. Take structs1.rs for instance and its hypothetical alternative:

structs1.rs
struct ColorClassicStruct {
    // TODO: Something goes here
}

struct ColorTupleStruct(/* TODO: Something goes here */);

#[derive(Debug)]
struct UnitStruct;

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn classic_c_structs() {
        // TODO: Instantiate a classic c struct!
        // let green =

        assert_eq!(green.name, "green");
        assert_eq!(green.hex, "#00FF00");
    }

    #[test]
    fn tuple_structs() {
        // TODO: Instantiate a tuple struct!
        // let green =

        assert_eq!(green.0, "green");
        assert_eq!(green.1, "#00FF00");
    }

    #[test]
    fn unit_structs() {
        // TODO: Instantiate a unit struct!
        // let unit_struct =
        let message = format!("{:?}s are fun!", unit_struct);

        assert_eq!(message, "UnitStructs are fun!");
    }
}
structs1.rs with "output"
struct ColorClassicStruct {
    // TODO: Something goes here
}

struct ColorTupleStruct(/* TODO: Something goes here */);

#[derive(Debug)]
struct UnitStruct;

fn main() {
    // TODO: Instantiate a classic c struct!
    // let green =
    println!("green.name = {}", green.name);
    println!("green.hex = {}", green.hex);

    // etc.
}

I don't think it works quite as well, since the learner has no clue what the value of green.name should be until they get the program to compile and see the Expected/Actual error message. And it'll be way harder to reach that point without the clue on what the fields of ColorClassicStruct should be. I suspect we could blame the exercise itself. Others, like if1.rs, might work better.

What I do think it works great is non-expected output exercises. Most of the "compile" exercises felt a bit anti-climatic. Once you get the program to compile, you can still play with it thanks to I AM NOT DONE, but you don't see the results of your tweaks. Even if we don't adopt mode="output", I think the default behavior of "compile" should be this.

So, summary of options:

  1. Scrub this, make "compile" always print output.
  2. Keep this PR as it is. It might be useful in future exercises, or maybe you have a strong opinion that some mode = "test" exercises should be converted.
    • If we keep it, we could still make "compile" always print as in (1) and
      reserve mode = "output" for mandatory expected output (i.e. change Mode::Output(Option<String>) to Mode::Output(String)).

src/verify.rs Outdated
@@ -101,11 +137,21 @@ fn prompt_for_completion(exercise: &Exercise) -> bool {
Mode::Compile => "The code is compiling!",
Mode::Test => "The code is compiling, and the tests pass!",
Mode::Clippy => "The code is compiling, and 📎 Clippy 📎 is happy!",
Mode::Output(..) => "The code is compiling and the output is correct!",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message doesn't make sense for Mode::Output(None) 🤦‍♀️

@@ -70,6 +70,26 @@ fn run_single_test_success() {
.success();
}

#[test]
Copy link
Contributor Author

@jrvidal jrvidal Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't there tests for failure cases?

The behavior of rustlings run for a mode="output" exercise with expected output is to simply run the program without running the "test harness". So I can't actually test the test harness itself 😞 I suppose this could be an argument in favor of actually running the comparison on rustling run.

@@ -70,6 +70,26 @@ fn run_single_test_success() {
.success();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fmoko One little detail that I noticed: the verify_all_failure test case misled me a bit, since it only tests that some exercise fails to verify. Should we change the name?

src/verify.rs Outdated

match comparison_result {
Ok(_) => {
success!("Successfully tested {}!", exercise);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-added this line, a regression introduced in #271 🙈

@shadows-withal
Copy link
Member

I might be a month late, but I think the first option sounds good for now. How do you wanna continue with this, make the changes in this PR or open a new one?

@jrvidal
Copy link
Contributor Author

jrvidal commented Apr 5, 2020

I can make the changes in this PR 👍

When running "compile"-mode exercises in interactive `verify` mode,
we print their output when we prompt the learner if they want to
continue. This improves the "experimentation" experience, since
trying different things does produce a visible change.
@shadows-withal
Copy link
Member

Looks good now! I'm trying to figure out how we wanna release this... it's definitely a breaking change, and there's some SemVer minor things laying around on master, so we could either do a minor release and then a major one a bit after or just coagulate all of it into a major release. What do you think?

@jrvidal
Copy link
Contributor Author

jrvidal commented Apr 7, 2020

Hm, usually I'd be inclined to push a minor and then a major, so people can avoid breakage if they want. However, given the way I assume most people consume rustlings (they clone it, work on it for a while, and probably never update the repo), I think it'd be fine to just release a major.

@shadows-withal shadows-withal merged commit 71d3125 into rust-lang:master Apr 7, 2020
@jrvidal jrvidal deleted the output-mode branch April 7, 2020 09:28
pedantic79 pushed a commit to pedantic79/rustlings that referenced this pull request Apr 11, 2020
ppp3 pushed a commit to ppp3/rustlings that referenced this pull request May 23, 2022
dmoore04 pushed a commit to dmoore04/rustlings that referenced this pull request Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants