Skip to content

Conversation

oreinert
Copy link
Contributor

@oreinert oreinert commented Dec 3, 2022

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:

  • Root access is required to install the dependencies under /usr/local/.
  • The process of setting up a development environment (locally) is cumbersome.

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:

  • Anyone just wanting to use homeshick can clone it as usual, which will produce the same result as before, except some additional metadata.
  • Anyone who wants to run test cases locally can just run git clone --recurse-submodules ... (or git submodule update --init on an existing repository) to get a complete setup where running bats test/suites works as expected.
  • If and when the dependencies are changed, it is not necessary to change the runtime environment.

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

@oreinert
Copy link
Contributor Author

oreinert commented Dec 3, 2022

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

@andsens
Copy link
Owner

andsens commented Dec 3, 2022

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:

version_compare "$GIT_VERSION" 1.6.5
if [[ $? != 2 ]]; then
git_out=$(cd "$repo" && git submodule update --recursive --init 2>&1) || \
err "$EX_SOFTWARE" "Unable update submodules for $repo. Git says:" "$git_out"
else
git_out=$(cd "$repo" && git submodule update --init 2>&1) || \
err "$EX_SOFTWARE" "Unable update submodules for $repo. Git says:" "$git_out"
fi

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
Copy link
Contributor Author

oreinert commented Dec 3, 2022

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
Copy link
Owner

andsens commented Dec 3, 2022

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
Copy link
Contributor Author

oreinert commented Dec 3, 2022

I agree about hardcoding exceptions.

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

load /usr/local/bats/support/load.bash
load /usr/local/bats/assert/load.bash
load /usr/local/bats/file/load.bash
load ../bats/lib/support/load.bash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an if [[ ! -d ../bats/lib/support ]]; then printf "bats libraries missing, run get_bats_lib.sh "; exit 1; fi here. Then people don't have to consult the documentation when a loading error occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That turns out to be a bit awkward to do.

The path given to the load function is relative to the test being run, whereas the path tested for in such a statement is relative to $PWD. Which makes the code look odd because there will be a mismatch between the path tested and the one loaded. Are you sure you want that?

@andsens
Copy link
Owner

andsens commented Dec 4, 2022

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 :-)

@andsens andsens merged commit 00c9206 into andsens:development Dec 4, 2022
@oreinert
Copy link
Contributor Author

oreinert commented Dec 4, 2022

Cool - thanks.

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

@oreinert
Copy link
Contributor Author

oreinert commented Dec 4, 2022

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

@andsens
Copy link
Owner

andsens commented Dec 4, 2022

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

p.s.: Try git clone [email protected]:andsens/homeshick.wiki ;-)

@oreinert
Copy link
Contributor Author

oreinert commented Dec 4, 2022 via email

@andsens
Copy link
Owner

andsens commented Dec 4, 2022

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 oreinert deleted the test-deps-as-submodules branch March 26, 2023 11:52
@oreinert
Copy link
Contributor Author

oreinert commented Apr 10, 2023 via email

@andsens
Copy link
Owner

andsens commented Apr 14, 2023

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

@andsens
Copy link
Owner

andsens commented Apr 14, 2023

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

@oreinert
Copy link
Contributor Author

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants