pijul_org / pijul

#74 Various commands default to master instead of the current branch

Opened by joeneeman, on May 7, 2017
Closed
joeneeman commented on May 7, 2017

Several commands (e.g. push, fork) default to the master branch if no argument is given. Other commands (e.g. diff, record) default to the current branch. This difference is surprising to me: I would expect everything to default to the current branch.

pmeunier commented on May 7, 2017

This is indeed a bug. Feel free to submit a patch! You can ask anything you need to do so.

joeneeman commented on May 8, 2017

Sure, I can do that. I'll let you know if I get stuck.

joeneeman commented on May 10, 2017

Ok, I'm not exactly stuck, but I'd like an opinion: how do you feel about moving the integration tests into an external tool? The reason I ask is that I was refactoring the various commands to try and pull out common code, and in doing so I broke lots of test code since it calls commands from rust instead of using the public CLI interface.

Anyway, I tried out some options and bats (https://github.com/sstephenson/bats) looks promising. It's MIT licensed and small enough to just import in-tree. I rewrote a couple of tests in bats just to get the hang of it, and I think they look pretty good:

@test "info in repo" {
pijul init
run pijul info

[ $status -eq 0 ]
[[ ${lines[0]} =~ ^Current\ repository\ root: ]]
[[ ${lines[1]} =~ ^Current\ branch: ]]
}

@test "info out of repo" {
run pijul info

[ $status -eq 1 ]
[[ ${lines[0]} == "error: Not in a repository" ]]
}

@test "add grandchild" {
pijul init
mkdir subdir
touch subdir/file.txt
pijul add subdir/file.txt
run pijul record -a -m msg -A "me <me>"

[ $status -eq 0 ]
[[ ${lines[0]} =~ ^Recorded\ patch ]]
}

What do you think? Shall I convert the rest?

pmeunier commented on May 10, 2017

Hi!

So, we used to have only bash scripts in the beginning, and bats definitely looks like a big improvement over that. I think it would be great to start having CLI integration tests.

I'd like to keep both kinds of tests, though, because libpijul doesn't have its own tests, because it needs files to work on, and those are typically supplied by pijul. Some of the tests we already do, or want to do in the future, particularly on libpijul, require Rust code to explore the data structure produced by the algorithms.

joeneeman commented on May 11, 2017

Right -- I didn't mean to suggest that tests in rust are bad, just that some (most?) of the existing tests look like they would be easier to write in shell. I've made a start at converting them (see my pending patch), and I'll keep going as long as I find more tests that make sense to convert.

pmeunier commented on May 19, 2017

Ok, giant messup on my side here, I'm super sorry, and I promise to start writing backup tools for the Nest.

Your contributions to solve this have been merged, but due to the conflict accident, I had to reboot history.

I didn't accept your contribution for bats though, because it deleted lots of tests. I believe it would be nice to have both tests, feel free to resubmit a patch only adding the bats tests.

(sorry, and thanks a lot for your help)

joeneeman commented on June 1, 2017

Ah sorry, I just read this now (nest's notification emails seem a bit sporadic). It looks like the patch for this didn't go through, but it's ok because I had a backup, which I've now pushed. Unfortunately, I just read what you wrote about tests, and the patch I sent yesterday would require a total rewrite of all the tests, since it removes most of the Param structs.

I can't figure out how to retract a pending patch, but I'll rethink this approach.