Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit testing #102

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Unit testing #102

wants to merge 4 commits into from

Conversation

pwall2222
Copy link

  • Added unit testing
  • Added separate file for functions
  • Setup of the git submodules is made with:
git submodule update --init
  • Test can be run within the repo with:
./test/bats/bin/bats test/test.bats
  • Uses bats and this are the docs
  • Solves Write tests #59 year long issue
    It's a huge paradigm shift from the one script to rule them all, but the only way to do unit tests is if we have units and functions seem the smallest units of shell scripting appropriate to test.

@pwall2222 pwall2222 mentioned this pull request May 10, 2023
@cpina
Copy link
Owner

cpina commented May 19, 2023

Thanks very much for the MR! I will try to review it soon (weekend or next week). First look is that is good, just need time to read it properly :-)

@pwall2222
Copy link
Author

@cpina You seem very busy, and I don't see a lot of movement in the repository would you be interested on adding me as a mantainer?

@cpina
Copy link
Owner

cpina commented Aug 15, 2023

This PR felt into other things that I've been doing.

I will have a better look soon. Thanks again for the PR, I should have acted (merge, feedback, etc.) sooner.

When I had a quick look in May I thought that I was in a catch-22 situation: I don't want to change the code without tests, but to implement the tests, in the PR, the code is changing :-) (and I really want to avoid regressions). I've had a look now and it's not as different as I had initially thought.

Regarding maintainer: I'd like to make the repo more sustainable, so I'll think about it (I have some concerns either way). Let's see first where I get with the PR :-) Thanks for offer your help though (and for the good PR with the needed tests and re-factoring).

@pwall2222
Copy link
Author

I will have a better look soon. Thanks again for the PR, I should have acted (merge, feedback, etc.) sooner.

No worries (I always get ghosted on PRs), and forgetting to do things is basically my thing so it's truly alright.

I've had a look now and it's not as different as I had initially thought.

I have purposefully made it as similar as the original as I could just splitting it into functions so it could be tested, further more if you don't like the changes that I propose (that are truly little over at a2e083e ) you can just throw away that commit.

Regarding maintainer: I'd like to make the repo more sustainable, so I'll think about it (I have some concerns either way). Let's see first where I get with the PR :-) Thanks for offer your help though (and for the good PR with the needed tests and re-factoring).

It's good to have concerns (trust, communication, etc), but being a lone maintainer is heavy, so even if it's not me (because you don't know me or trust me) you should look into having another maintainer.

I hope I can hear your feedback soon.

@pwall2222
Copy link
Author

@cpina RFC

@cpina
Copy link
Owner

cpina commented Sep 6, 2023

My thoughts at the moment:

  • I really like the approach that you took for the unit tests. If I started the action now I would take this approach!
  • I feel uncomfortable with the "catch 22" situation that generates: change the code to add unit tests, but I wanted to add tests to change the code (in Write tests #59 I talk about tests in general, not unit tests to avoid changing the code)
  • Because of #live and #work I prefer to not commit now to make structural changes in the action. I know that is unlikely that will break things, but it's possible. The action is quite used and I want to avoid regressions (and I know that many people use @main instead of the tagged versions). I don't want to have to fix things if I can avoid breaking them (and, when GitHub changed things causing regressions, I fixed them as quickly as I could).
  • I don't think that I should add a maintainer at this point

Side topic: My plans for when I get into this: I want to add an explanation (in the FAQ) in the docs on how to use .gitignore (as mentioned in some issue or MR) to avoid copying / overwriting files. I thought that it was a clever way, and .gitignore has many, many features on includes and excludes. Adding options to the action for this would be endless (exclude a directory, include subdirectories, exclude files..), if it can use .gitignore we can cover many things (which I can if I remember correctly).

I also want to add a list of related github-actions (other actions that serve the same or similar purpose). Not all of them can do everything, and it would be useful to have a bit of a list for when one of them does not fit a use case.

@pwall2222 : an idea is that you fork this one, merge your MR and I add yours (and others!) in the docs. If you do: ping me here and I'll add this section in the docs. I guess that this is probably not your desired way (else you would have already done) but I think that's a way to move forward.

Perhaps, when I have time to write integration tests to ensure that nothing gets broken then I even merge it back? Who knows!

For now I'm happy to leave the MR open because I still find it interesting!

Feel free to get in touch for any comments or thoughts!

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