Would A Test Suite Be Useful?

[acviana]

Is your feature request related to a problem? Please describe.

No, not really.

I was reading the source code and I noticed that there are no tests. I'm a big fan of this project and I thought a testing suite might be a useful way to help out.

I searched the project issues and couldn't find any open or closed issues that mentioned "test[s]" so I assumed this has not be previously discussed. I wanted to do some research on what a Bash testing framework could look like and it how easy it would be to implement one for this project.

Describe the solution you'd like

After doing some research I came to the conclusion that it's probably not worth the effort to add tests to the project.

I think it would require a substantial change to the project structure and there are few places where there is complex logic to test.

I'm sharing my notes in case folks with more experience with the project disagree.

Describe alternatives you've considered

First of all, for context I'm a Python developer. As a result, I'm might be missing some context on how to write idiomatic Bash scripts. If so, my bad. Here are the major points I thought about:

Testing Framework: After some looking around on Stack Overflow it seems like the bats-core project is one of the more modern Bash testing frameworks so I used this in my local tests.

Importing Functions for Testing: Because calling padd.sh without any arguments runs the script you can not source or load the project to get access to the individual functions for unit testing.

I think the way to get around this would be to preserve padd.sh as wrapper but move all the functions into something like padd_lib.sh that defines, but does not execute, all of the functions. padd.sh would then call source padd_lib.sh but otherwise remain exactly the same.

This change should be transparent to end-users -- but is of course risky.

Mocking Functions: Several of the functions in this project make system calls so we would need a way to mock those calls. This blog post suggests the best way to do this is to basically overload the bash built-in commands with local functions in the testing namespace.

This seems doable but kind of begs the question of what we would be mocking / testing:

What to Test: It seems like 95% of the code in this project is string manipulation and system calls. This leaves relatively little remaining internal logic to test and a substantial number of complex system calls to mock out. It's possible that most of the corner cases were explored long ago and this project is in a stable enough that the refactoring I'm proposing is more likely to introduce errors than just leaving the code untested.

Disruption to Open PRs: Because this is a major refactor (at least in terms of the project structure) this will break all the outstanding PRs. Some of those PRs will certainly not be updated due to a lack of momentum and the changes will be lost to the project.

Additional context

I know this is a little bit of a weird "feature request" but I thought I would put it out there in case anything useful comes out of the discussion.

In any case thanks for the great project!

[DL6ER]

Just a few quick comments:

1. I don't think it's the right time to write tests as PADD is going to be rewritten this year when FTL v6.0 matures. The change will be to go from the current telnet API to a new REST API returning JSON. If there are also tests that need to be rewritten, it doubles (or manifolds) the effort needed to port PADD to the new API.
2. We already use bats in the FTL project (see https://github.com/pi-hole/FTL/blob/master/test/test_suite.bats) so this is a well-known tool in our project
3. There is no need to split padd.sh into a function and a wrapper file. It could simply be done like in the pythonic world where you define everything in a main() function that is only being called when you detect that there is an end-user running the script directly. It'd be possible to define something like padd.sh test which allows sourceing without any start of interactive parts.

[acviana]

Thanks for taking the time to explain all that. Based on your comments I think we can close this ticket as there is nothing to do at this time and I don't think this discussion is essential for a future testing suite.

A few other comments:

1. I wasn't aware of the move to a new API with FLT v6.0. That's important context, I'll keep an eye on that release.
2. This is very helpful, I've never seen a testing suite fully implemented in bash!
3. That makes sense, I think this would be the equivalent of the if __name__ == "__main__": statement in Python. I didn't think to check if there was an equivalent in Bash.

[yubiuser]

Based on your comments I think we can close this ticket as there is nothing to do at this time and I don't think this discussion is essential for a future testing suite.