Add tests and an example solution to Rust training Project 1

[sticnarf]

This pr is based on #7 .

The structopt extension is not included yet.

I don't know how to test the cli in integration tests, so I only write tests for KvStore.

The clap get_matches function uses os_args, which I cannot easily hack.

Another way is to use get_matches_from_safe and pass args in, but that will make the signature of our main function horrible to beginners.

assert_cli or assert_cmd should work too. But then I cannot think of the advantage of placing the user interface into the library.

[sticnarf]

I am not so confident about the answer to question D and E.

I thought it unimportant whether to run an empty test or not. Running an empty test just looks more consistent. Still, I wrote something as the answer.

As to question E, I must admit I didn't seen this pattern before, and now I still don't see how we can benifit from this pattern.

[brson]

It might be worth mentioning that cargo decides what to do with all three of these files implicitly, that lib.rs is compiled into a library because cargo recognizes src/lib.rs specially; that main.rs is compiled into an executable because cargo interprets any rust file in src/bin/ as an executable; tests.rs is compiled into test binary via cargo test because cargo treats any source file in ... etc.

[brson]

Good answer. It makes me think we should just set up the project differently, with main.rs in src/, and get rid of the question. What do you think?

[sticnarf]

Yes, I think we can just use src/main.rs but leave the question to be "modify the binary name in different ways".

[brson]

"cargo tet run unit tests" -> "... runs unit tests"

[brson]

The main reason having these empty test suites can be problematic is because they each need to be compiled and linked, which becomes more and more time consuming as a project grows. To run the tests in a library, for example, requires building the library once as a library, so other crates can link to it, and then a second time, as an executable.

Your first sentence captures that, but not in much detail. The second sentence I don't understand.

Can you do a little rework here?

[sticnarf]

I misunderstood the question. I will do the rework.

[brson]

Nice. I wasn't actually sure that doc tests could be disabled.

[brson]

Your last sentence here seems most important to me. I think it needs more explanation about what "waste of space" means. The UI is going to have dependencies that are not needed by the library, so any library consumers are going to be forced to link to UI code they don't need, and that will increase their compilation time and binary size.

Can you move the last sentence to the top and clarify it?

[brson]

Hey @sticnarf!

This all looks great, and I'm glad you've found some areas where things don't really fit together. It seems like you've uncovered a few issues that need to be reconsidered:

• Hyper has no synchronous API, making the proposed synchronous networking project not work.
• The way I've laid out the main.rs file may not be the best. It may be better to put it somewhere where the name is automatically "kvs", cutting out the question B.
• Putting the main function in the lib is a bad pattern generally, and it doesn't actually solve the problem I intended it to (making the CLI testable). A different solution here would eliminate question D.

That last issue is one I've considered in the past, but it seems to have slipped my mind when I wrote the project.

Offhand I only see a few solutions:

• pass the args to the library main function. This doesn't seem all that bad to me as long is we explain why we're doing it. I think though that since putting main in the lib is an anti-pattern, doing it here sets a bad precedent, so I'd prefer to find a different solution.
• write a separate makefile or python script for testing the cli. I strongly want to keep our tests inside the rust test framework, since that helps teach rust, and since having another script complicates the instructions.
• complicate the workspace such that the cli is a dependency of a test suite, have the tests find the kvs bin in target/ and run it from inside test cases. I don't think this can be done without converting the project to a workspace and adding another library, which is a lot to explain.

Upon investigation, it looks like cargo will compile the projects bins before it runs tests, so that last solution seems viable.

I think we should land our outstanding PRs before trying to solve these problems though.

The code commit looks great.

---

I am not so confident about the answer to question D and E.

I left inline comments.

I thought it unimportant whether to run an empty test or not. Running an empty test just looks more consistent. Still, I wrote something as the answer.

I left a comment describing what I see as the big downside (duplicated compiling and linking). If you don't find that insightful then we should probably delete the question.

As to question E, I must admit I didn't seen this pattern before, and now I still don't see how we can benifit from this pattern.

It's kind of a cute trick, but it's definitely an anti-pattern in most code. I worded the question too strongly by saying it "often a reasonable thing to do".

Treating a crate as both a library and executable does come in handy sometimes though. An obvious example is the stock test runner, which instruments your library with a synthetic main function and then compiles it into a binary.

One has to do something similar manually when using e.g. the criterion benchmark tool - when you use it you define main yourself, so putting criterion benchmarks in a library requires writing a main function.

Still, I'm convinced this is too esoteric a subject, and we can avoid talking about it by changing the cli test setup.

---

So I left a few change requests. Please do them. I'll move on to project 2 so that Thursday you have something to work on.

If you have solutions for any of the problems above I'm glad to hear them. How long did it take you to write the solutions? Should we expand the scope of this project, maybe with CI setup?

[sticnarf]

I have done the rework you mentioned. Please take a look again.

I agree we should add CI setup. I find PingCAP projects use CircleCI mostly, that may be a good choice IMO. A problem I can think of is that perhaps it's hard to evaluate this task.

---

Writing the answers and code took me about four or five hours. That's longer than expected because I got stuck in some problems I have no idea to solve (as you saw above) and was struggling with English writing which I am not very comfortable with yet. However, I feel happy as I think all these work is beneficial to me.

[sticnarf]

I think it's still possible for us to test the cli using the rust test framework without changing our project structure.

For example:

#[test]
fn cli_no_args() {
    let output = Command::new(binary_path())
        .output()
        .expect("failed to execute kvs binary");
    assert!(!output.status.success())
}

// Path to kvs binary
fn binary_path() -> PathBuf {
    // Path to cargo executables
    // Adapted from https://github.com/rust-lang/cargo/blob/485670b3983b52289a2f353d589c57fae2f60f82/tests/testsuite/support/mod.rs#L507
    let mut path = current_exe()
        .ok()
        .map(|mut path| {
            path.pop();
            if path.ends_with("deps") {
                path.pop();
            }
            path
        })
        .unwrap();

    path.push("kvs");
    path
}

[sticnarf]

Should the example solution contain the extension code? I thought it inappropriate not to show the common clap code and I didn't know where to put the structopt code so I didn't do that.

[brson]

Should the example solution contain the extension code? I thought it inappropriate not to show the common clap code and I didn't know where to put the structopt code so I didn't do that.

Good question. For now I think no, per your comments. We can reconsider later.

In #7 @nrc found one of the other questions unnecessary, so I think this idea of putting questions in the project is not working out and we should just remove them.

Since we need to start on project 2, and I want to be able to see the existing content with the new content, I'm going to merge this now. Any additional work you still need to do on it can be in another PR. I hope that's ok with you.