andsens / homeshick

git dotfiles synchronizer written in bash
MIT License
2.11k stars 145 forks source link

Provide test dependencies as submodules #213

Closed oreinert closed 1 year ago

oreinert commented 1 year ago

It used to be that homeshick could be tested just by cloning the repository and running bats test/suites. Unfortunately, since the introduction of the 3 bats libraries, this is no longer the case.

The wiki specifies how the test dependencies should be installed to make it work, but there are some disadvantages to this approach:

With this PR, I propose turning the bats dependencies into Git submodules of homeshick itself. This is also the approach suggested by the bats-core developers. The advantages of this are:

In case this PR is accepted, I volunteer to change the wiki page about testing accordingly, too.

oreinert commented 1 year ago

I guess .devcontainer/Dockerfile ought to be adjusted accordingly, too?

andsens commented 1 year ago

That's awesome Olav! Unfortunately it would result in everybody cloning those modules because homeshick is tracked through homeshick itself and when pulling it does this:
https://github.com/andsens/homeshick/blob/dbe62897ec1a24b82ca47412cfe4180f9225b001/lib/commands/pull.sh#L20-L27

I think a better way to do it would be to auto-download and extract the packages with e.g. wget -O- http://... | tar -xvzf

oreinert commented 1 year ago

How do you feel about exempting homeshick itself from the recursive submodule treatment in pull.sh?

The rationale being that someone who wants to develop on homeshick probably wouldn't use the copy managed by homeshick itself.

andsens commented 1 year ago

I really don't like hardcoding exceptions like that. I think the download method is the way to go. Especially because we already do it for the varying bash versions. (Like this). Then you can just run the test script without having to do anything.

oreinert commented 1 year ago

I agree about hardcoding exceptions.

Scrapped the old approach, and replaced it with a download script, as suggested.

andsens commented 1 year ago

Looks great! I think the $(dirname "${BASH_SOURCE[0]}") looks fine. It's a very common thing to do in bash scripts. People familiar with that should have no trouble understanding what is going on.

LGTM, merging. Thank you very much for the contribution Olav. I'll try to find the time to test everything in the testing branch and then merge to master :-)

oreinert commented 1 year ago

Cool - thanks.

It would also be nice if you could make a new release version once you have merged this to master.

oreinert commented 1 year ago

Finally, here's my suggestion to how to update the wiki page Testing.md.txt

andsens commented 1 year ago

Looks great! You can go ahead and put that in.

p.s.: Try git clone git@github.com:andsens/homeshick.wiki ;-)

oreinert commented 1 year ago

I tried that, and I found it a bit lacking - I can't push to the wiki repository, not even a new branch. Seems to me you're the only one who can do that.

Besides, the page should not be updated before you've merged all the changes to master. That's why I sent you an updated .md file as an attachment, so you can do the update at your own leisure.

On Sun, 2022-12-04 at 08:13 -0800, Anders Ingemann wrote:

Looks great! You can go ahead and put that in. p.s.: Try git clone @.:andsens/homeshick.wiki ;-) — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.>

andsens commented 1 year ago

Ah, gotcha. I thought everybody was able to.
I pushed the changes already. The assumption is that you develop on testing or development. It's better the testing docs for those are up to date, than the ones for master.

oreinert commented 1 year ago

Hi, it's been a while now, and I'm wondering how testing is going? I would like to cut a new version of the openSUSE package soon...

On Sun, 2022-12-04 at 03:29 -0800, Anders Ingemann wrote:

Looks great! I think the $(dirname "${BASH_SOURCE[0]}") looks fine. It's a very common thing to do in bash scripts. People familiar with that should have no trouble understanding what is going on. LGTM, merging. Thank you very much for the contribution Olav. I'll try to find the time to test everything in the testing branch and then merge to master :-) — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

andsens commented 1 year ago

Heyo! Testing went well, I just completely forgot to merge the changes into master. I have created a release for you :-)

andsens commented 1 year ago

btw. do you have any usage statistics for the package. I'd be curious to know how many openSUSE installs there are.

oreinert commented 1 year ago

I have no idea about the usage of the openSUSE package. As far as I know, a mechanism for creating consolidated download statistics for individual packages across all mirrors doesn't exist (for openSUSE).