Add some CLI tests and workflow #18
Conversation
|
As said in #13, as the start, this PR will add some basic CLI interface tests and workflow to test it on new pull requests. |
|
Excellent, thanks! It's getting late here, so I'll look at this in more detail tomorrow. One quick high-level request: would you try splitting this up into a series of commits for the different pieces of what it does? That would help me understand the dependencies between the different parts of this PR, which will help me better understand and review it. For example, I think something like the following sequence will work:
But I could easily be missing something — please vary that sequence however seems appropriate. The idea is that each commit should work correctly on its own, and then later commits can build on that. |
There was a problem hiding this comment.
OK, I've read through the test changes. There's one issue described below; otherwise, they look good! Thanks again.
I've made a revision to this branch that splits out the tests as their own commit at the start, and also drops the change that has the issue below. I'll push that back to your PR branch in a moment.
How about splitting out the workflows and Makefile as another separate PR? Then I'll be glad to go ahead and merge that first commit that adds the tests.
I also have some ideas for how to refactor the new tests so they're simpler and more concise to write, which I think will also make it easier and more fun to write additional tests. After these tests are merged, I can try pushing some more commits to make those refactors.
test/test.rs
Outdated
| fn get_exec_path() -> PathBuf { | ||
| // TODO is there no cleaner way to get this from Cargo? | ||
| // Also should it really be "debug"? | ||
| let target_dir: PathBuf = env::var_os("CARGO_TARGET_DIR") | ||
| .unwrap_or_else(|| OsString::from("target")) | ||
| .into(); | ||
| target_dir.join("debug").join("toml") |
There was a problem hiding this comment.
So as the comment here says, I'd be glad to find a way to simplify this bit of code. One thing this PR does is to replace it with something much simpler, namely the constant string "toml".
But when I run cargo test from the project directory after this change, these tests fail:
Running test/test.rs (target/debug/deps/test-f800c5db31d9f226)
running 3 tests
test integration_test_help_if_no_args ... FAILED
test integration_test_cmd_set ... FAILED
test integration_test_cmd_get ... FAILED
failures:
---- integration_test_help_if_no_args stdout ----
thread 'integration_test_help_if_no_args' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', test/test.rs:10:56
---- integration_test_cmd_set stdout ----
thread 'integration_test_cmd_set' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', test/test.rs:59:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- integration_test_cmd_get stdout ----
thread 'integration_test_cmd_get' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }', test/test.rs:30:10
It looks like in CI this would work because before running tests, it builds and then installs into /usr/local/bin/. But I think it's important to arrange things so that when you're developing changes, you can quickly run the tests on them with a simple command like cargo test. I believe we can accomplish that by continuing to use this get_exec_path function.
Signed-off-by: bin liu <[email protected]> [split from larger commit; new commit message] Signed-off-by: Greg Price <[email protected]>
|
OK:
|
|
And done. Thanks again @liubin! Eager to see that split-up version of the CI, release, and Docker changes, whenever you have a chance to do so. |
This commit adds
And the release will be triggered by pushing a tag:
Here is the created release: https://github.com/liubin/toml-cli/releases/tag/v0.2.1
Signed-off-by: bin liu [email protected]