radicle-dev / radicle-surf

A code browsing library for VCS file systems.
Other
32 stars 11 forks source link

Testing strategy #176

Open FintanH opened 3 years ago

FintanH commented 3 years ago

After reading this article https://matklad.github.io/2021/02/27/delete-cargo-integration-tests.html, I'm thinking that some of the testing organisation could be changed.

For example, the current layout is generally a mod test { /* test cases */ }. In the article, it's recommended to use mod test with a test.rs file so that the compiler doesn't think the original module itself has changed.

There are a lot of doc tests, which the article says are slow, but I think since it provides a good view of the API and what the functions do they should stay.

A set of integration tests might be a nice addition, but no particular plan in what flows could be tested.

uint commented 3 years ago

I'm new and only starting to dig into this project so take me with a grain of salt, but would it make sense to also revisit the whole thing with having to set up test fixtures (git refs) by external script before running cargo test?

It doesn't feel right that I can't clone this project, run cargo test and have it Just Work©. To me, in an ideal world the tests themselves would ensure the right environment is there (as much as possible).

FintanH commented 3 years ago

Thanks for taking the time to look around @uint :blush:

but would it make sense to also revisit the whole thing with having to set up test fixtures (git refs) by external script before running cargo test?

I'm definitely for discussing this! The fixtures testing has always left something to be desired. I tried to make it as seamless as possible by using a build.rs file to set up the submodule. That became too much of a headache. For whatever reason, it wouldn't update the refs properly and I ended up having to incorporate an env var. So I just ditched that method.

What kind of approach were you thinking of? Just keep in mind that it needs to work on the unit tests as well as the doc tests, and I'd hope to not have any noise in the doc tests, if possible.

uint commented 3 years ago

@FintanH I'm guessing since you're using submodules to keep that git repo around, it's not really possible/straightforward to keep other refs in it, right? Git probably just checks out one tree and that's it, and that's why you add other refs dynamically.

Have you considered ditching submodules and packaging up the whole git repo in a tarball, along with all the refs needed for tests? Then the only thing tests need to do for prep is untar the thing to some tempdir. I'm not sure about doctests, but if we have a function ensure_git_platinum() or something that does the prep, we could include that line in a doc example and hide it. I guess that still creates some noise in the source code.

FintanH commented 3 years ago

I'm guessing since you're using submodules to keep that git repo around, it's not really possible/straightforward to keep other refs in it, right? Git probably just checks out one tree and that's it, and that's why you add other refs dynamically.

Ya, so I've tried pushing a namespaces branch to GitHub and if you clone the project again that branch will disappear. It's necessary for us to be able to browse namespaces.

Have you considered ditching submodules and packaging up the whole git repo in a tarball, along with all the refs needed for tests?

Initially, I believe radicle-surf had the git repository living in its repo (it's been a while :sweat_smile:). We wanted to be able to re-use a template repository in-between radicle-surf and radicle-upstream to have some parity in how they were progressing and being tested.

I'd be fine with moving back to having a dedicated repository in radicle-surf if it means an easier dev experience for ourselves and contributors :)

Is there any advantage to tarballing the project? I'm curious because it seems like a bit of overhead to update it (if needs be) and to untar it for each test.

uint commented 3 years ago

Fair warning - I'm probably a bit biased against git submodules, just because I feel like they tend to complicate things. I'm just playing devil's advocate here. I'm not alone, though!

Initially, I believe radicle-surf had the git repository living in its repo (it's been a while sweat_smile). We wanted to be able to re-use a template repository in-between radicle-surf and radicle-upstream to have some parity in how they were progressing and being tested.

I think that kind of parity could still be achieved with a simple script that updates git-platinum by checking it out from https://github.com/radicle-dev/git-platinum (and possibly adding the refs if needed?). When an update is needed: run script, commit, forget.

If some refs are missing after a plain checkout, I think there are arguments to play around with that might help. Maybe.

Is there any advantage to tarballing the project? I'm curious because it seems like a bit of overhead to update it (if needs be) and to untar it for each test.

So in the past I worked on something similar (a project that contained some kind of inner git repo as a test asset) and I vaaaguely remember git not liking such nested git repos very much. I could be misremembering. If it works as is, that's great!

FintanH commented 3 years ago

Fair warning - I'm probably a bit biased against git submodules, just because I feel like they tend to complicate things. I'm just playing devil's advocate here. I'm not alone, though!

They're a PITA, I know :)

So this all sounds good to me! Are you interested in taking this task on? Happy to help you out if you need it :blush:

uint commented 3 years ago

I can give it a go! Sounds like there'll be experimentation involved, but that's just dandy for me ;)

FintanH commented 3 years ago

You love to see it ;) Ping me if you need any guidance, but it sounds like you got this