Proposal for unit & integration testing #1

robertdfrench · 2021-10-10T05:14:56Z

There's a lot going on, and I haven't documented any of it. But the idea is to be able to do unit tests without relying on real git, since it could be slow, use the network, or impact local state.

So cargo test --lib will use mostly in-memory tests, reaching out to fake git binaries when necessary. The Integration tests (under the tests/ folder) will run against the real git binary, and may do things like authenticate to remote hosts or create / remove branches.

This is not really intended to be a PR, so much as a conversation starter around testing. I think we'll want to do some of this, but maybe not all of it.

There's a lot going on, and I haven't documented any of it. But the idea is to be able to do unit tests without relying on real git, since it could be slow, use the network, or impact local state. So `cargo test --lib` will use mostly in-memory tests, reaching out to fake git binaries when necessary. The Integration tests (under the `tests/` folder) will run against the real git binary, and may do things like authenticate to remote hosts or create / remove branches.

failing_git.sh

src/lib.rs

calebwherry

Finished review.

* Rewrote fake_git sh scripts in Rust * Moved all executables to src/bin & removed from TOML * Updated tests to use new exes

I wasn't 100% sure this would work at first, but you can split an `impl` block up over multiple sections. So since `with_path` doesn't need to be defined for the `Git` struct in application code, I moved it into the `test` module instead. If you don't do something like this, then marking `with_path` as private will generate a warning, since no other application code (even within its own module) uses it. Two alternative approaches: * Leave `with_path` where it is, but mark it as `#[cfg(test)]` * Change `new` to call `with_path`, so that `with_path` is used by something.

src/lib.rs

Invoking git either raises an `io::Error` or returns a `std::process::ExitStatus`, the `success()` method of which may return `false`. The new type, `GitError`, is designed to encompass both of these cases and should simplify error handling when interacting with git subprocesses.

Commented Cargo.lock from gitignore so that we have a record of why we did it. I think something like this is easy to forget so leaving it along with the original comment makes sense. Adding the version of Cargo.lock I get running on MacOs.

calebwherry

All latest changes and comments resolved. Need final pass on open items.

We can add build/release/platform info once we have CI set up (see #2).

robertdfrench · 2021-10-12T02:26:27Z

@calebwherry I like all your changes, I think I addressed the remaining concerns you had. Only thing left is figuring out what path to take with matching remotes other than "origin".

robertdfrench · 2021-10-12T02:27:18Z

Btw, here is the link to see what the updated README will look like: https://github.com/robertdfrench/git-pr/tree/pr-list/0

@calebwherry

As @calebwherry said in the original issue: > Using origin can cause issues with others not using that wordage. > Replace with "not slash" match as it can be anything. I tried to be clever and match hyphenated words, but then I tried to look up the actual definition of a remote name, and there does not seem to be one: https://stackoverflow.com/questions/41461152/which-characters-are-illegal-within-a-git-remote-name/41462742 So, it seems like the best thing to do is detect whatever isn't a slash, because we know those *can't* be allowed, since they are allowed in branch names (and therefore any ref with slashes in the remote or branch would parse ambiguously). Whatever else git tells us is obviously valid, since git is the authority here.

calebwherry · 2021-10-13T01:29:12Z

@robertdfrench Can you fix the merge conflicts? I assume just merge trunk is all that is needed? Once you do that, I'll merge this PR in!

robertdfrench · 2021-10-13T01:30:10Z

@calebwherry good catch, and luckily this gives us a chance to see whether our new CI pipeline passes!

GitHub checks out the repo in a detached head state. We use a trick noted in the task to fix it.

GitHub checks out the repo in a detached head state. We use a trick noted in the task to fix it. Try 2...

GitHub checks out the repo in a detached head state. We use a trick noted in the task to fix it. Try 3...

@calebwherry

Found by @calebwherry.

So we can commit!

robertdfrench requested a review from calebwherry October 10, 2021 05:14

robertdfrench marked this pull request as draft October 10, 2021 05:15

robertdfrench marked this pull request as ready for review October 10, 2021 05:15

robertdfrench changed the title ~~Draft: proposal for unit & integration testing~~ Proposal for unit & integration testing Oct 10, 2021