Test framework, fix 7-character callsigns not supported

[cbs228]

This PR adds cmocka-based unit tests to ardopcf.

I selected some simple functions that looked unsafe and wrote some tests for them:

• CompressCallsign(); and
• DeCompressCallsign()

In the process of writing these tests, I discovered that 7-character callsigns do not appear to work. The second commit in this patch series demonstrates this.

$ sudo apt install libcmocka-dev    # required prerequisite
$ make test
tests/test_ARDOPC:
[==========] Running 2 test(s).
[ RUN      ] test_compress_callsign
[       OK ] test_compress_callsign
[ RUN      ] test_decompress_callsign
[  ERROR   ] --- "CAFECAF" != "CAFECA"
[   LINE   ] --- tests/test_ARDOPC.c:56: error: Failure!
[  FAILED  ] test_decompress_callsign
[==========] 2 test(s) run.
[  PASSED  ] 1 test(s).
[  FAILED  ] 1 test(s), listed below:
[  FAILED  ] test_decompress_callsign

The spec requires support for three- to seven-character callsigns + SSID.

The HEAD commit in this PR proposes a fix. I am not sure if the fix is compatible with other ARDOP implementations. Tests verify that we have left six-character wireline representations unchanged.

These changes support the following eventual goals:

• Eliminate gcc 13 compiler warnings
• Eliminate unbounded string functions like sprintf() and strcat()
• Pass buffer sizes to all functions that produce variable-length output
• Add const-correctness guarantees where easy and appropriate

Closes #30

[pflarue]

Thanks for providing this. I haven't taken the time to give it the attention it deserves, but I will do so soon.

I'm currently in the middle of preparing several large changes to the develop branch which will remove some obsolete features, delete unused and unmaintained files from the repository, and remove some of the unreferenced code fragments from the files that remain. Unfortunately, these large changes also provide opportunities to unintentionally break useful remaining features. So, I want to stay focused on them until I think that they are ready to push to GitHub so that others can try them (to help identify mistakes I have made).

[cbs228]

I will happily rebase this when you're finished.

If you're planning to touch a bunch of code anyway, it may be a good idea to cleanup whitespace and line endings. It's easy to do, but it touches most of the code. See #52.

[cbs228]

Rebased on develop. I also added uppercasing and padding of compressed ASCII text as necessary in ASCIIto6Bit(), just in case the caller neglects to do this.

[cbs228]

This is ready for review, but I continue to mark it as draft as it is likely to prompt some discussion and may not be something we want to merge all at once. There probably needs to be dev documentation for the tests / testing framework, too.

In particular, I'd like to know:

• Is the use of an external / OS-packaged test framework acceptable? If not, I can vendor-in and build a less capable framework, like unity. These other frameworks have significant functionality gaps.

• Any objection to the overall approach of rewriting utility functions to be safer while preserving their internal APIs? My main focus at this point is eliminating unsafe string functions and misuse of buffers.

• Any general style comments?

[pflarue]

@cbs228, I appreciate your work on this, and will try to find time to look at it in the next few days.

I've been busy with other things and am just back from about a week away from Internet service, during which time my portable winlink/ardop station proved useful for communicating with the outside world.

So, please be patient.

[pflarue]

I like this approach to testing. It should allow us to implement a wide variety of tests, which will hopefully help us to identify and eliminate bugs in ardopcf. I also like that it introduces no significant additional complexity for someone wishing to build ardopcf, but who does not plan to run these tests.

While the contents of the pull request worked fine for Linux, and provide the simple instructions required to install cmocka, several changes to the Makefile are required for use with Windows. The attached Makefile works for both Linux and Windows. I also added some additional comments including instructions to install cmocka for 32 and 64 bit windows build environments.

Makefile.txt

The .txt file extension was added to Makefile so GutHub would allow it as an attachment. There is probably a better way to propose a change to a pull request here, but I don't know what it is. I'd welcome a comment suggesting how much changes should be better handled in the future.

In addition to the added comments and changes to this Makefile to work for both Windows and Linux, I also removed the reference to --lcmockery. This did not appear to be required. If this is indeed required, please explain and I will figure out and document how to also install it for Windows.

Some tests may be added which test OS-specific code. When that occurs we'll need to figure out how to handle them. For all other tests, I will plan to run them in each build environment used to produce releases. Thus, it is important that all test related code work on Windows as well as Linux.

Regarding the tests provided in ARDOPC/tests/test_ARDOPC.c and the changes made to the tested functions, I haven't yet looked at them closely. For the test cases, I would like to see comments in the test source files explaining why these test values are believed to be correct. Of course I'm referring to all tests to be included, not only these initial ecamples. Where appropriate, comments associated with test cases should reference the Ardop specification. For details missing from the specification, but which would impact compatibility with other Ardop implementations, the comments should reference the source code of or testing against one or more other implementations identified by name and version. In other cases, a brief description of how the test cases were manually arrived at may be appropriate. The intent of all such comments is to allow someone other than the person who wrote the comments to verify that what is being tested appears to actually be correct. Otherwise, it is possible that incorrect or ambiguous tests will be created.

Is there a traditional pattern to be followed regarding the location and filenames for these test files? If so, or if you would like to propose a pattern to be followed, please describe it here so that it can be added to the project documentation. Unless tradition dictates otherwise, I would like to suggest that we put the tests directory at the top level of this repository rather than within ARDOPC/. Since I anticipate also adding some additional non-cmocka tests to this repository, I'd like to further propose that these test source files be located in /tests/cmocka/ or something similar. Then other types of test code can be located in other subdirectories below /tests/. Comments?

[pflarue]

@cbs228, regarding your other questions from a few comments back:

I have no problem with rewriting of any functions to improve their safety/reliability as you described. I'm also happy to consider changes that do alter internal APIs if they improve the code. In the long term I'd like to reduce reliance on the large number of global variables used throughout this code, since these sometimes make it harder to track down interactions between different functions. This is just one example of the type of changes I would welcome.

If you plan to make these sort of changes to large or widely used functions that will require a lot of simultaneous changes to the source code, it might be useful to first create a new issue. That would provide an opportunity to discuss the impact of the changes, and would also notify me and anyone else working on unrelated changes of the portions of code that you will be working on. Knowing you are working on a portion of the code makes it easier to avoid conflicts requiring difficult rebase operations.

What do you think of a recommendation to create new test cases for functions before rewriting them? Use this to both confirm your understanding of what the function currently does, and perhaps to document problems within it. Then, after the rewrite these test cases (which may need to be restructured somewhat if the API changes) would verify that no unintentional changes were made and that any known problems were eliminated.

If following this approach, I'd like to see the initial test cases submitted as a pull request as early as possible so that I (and anyone else interested in doing so) can review them to look for mistakes or suggest additional edge conditions that should also be tested. If test cases are submitted simultaneously with changes to the function being tested, my approach to reviewing the pull request(s) would probably be to initially focus of the test cases, perhaps asking for or making changes to them, before reviewing the changes to the function being tested.

I'd also like to ask that plenty of comments be used in the code. Producing good code that makes ardopcf work better and more reliably is the primary goal. However, I'd also like this code base to be useful as a reference for someone who wants to develop something similar or use parts of it in another project. The better it is documented, the more useful it is likely to be for these other uses.

As far as other general style guidance, I don't have any suggestions at this time. The existing code seems to have a lot of variation in style. I think that this is partly because it began as a translation from another author's code in a different programming language. That said, if you'd like to suggest some style guidelines for future modifications, please create a new issue to initiate that discussion.

[cbs228]

There is probably a better way to propose a change to a pull request here, but I don't know what it is.

There are lots of ways:

• To propose small changes, use the Github Code Review process. See "Reviewing proposed changes in a pull request." Here you can comment directly on individual lines of code. You can also "Add a suggestion" to make inline changes. This is for small changes only.

• For more complex things that might require a review, it's probably better to just push it to another branch and say, "go look at my changes on branch XXXXX." You can do this informally or even with your own PR that targets my branch in my repo.

• I checked the "allow edits by maintainer" box, so you can just push more commits to my branch in my repo. That's fine for small, non-controversial changes.

I will try to integrate your Makefile changes on my end.

I also removed the reference to --lcmockery. This did not appear to be required

Oooh, you're right. Looks like the Debian libcmocka-dev ships a backwards-compatible library for Google's "cmockery," and like a dolt I linked it 'cause it was in the package.

Is there a traditional pattern to be followed regarding the location and filenames for these test files?

Location: varies. Some projects use a top-level src/ and test/ directory. Each subproject or target gets a subdirectory.

Makefile

src/
├─foo/
│ └─foo.c
└─bar/
  └─bar.c

test/
├─foo/          # has tests for src/foo/
│ └─test_foo.c
└─bar/
  └─test_bar.c  # has tests for src/bar/

Others might bundle the sources and tests for each subproject or target together.

foo/            # sources and tests for foo
├─Makefile
├─src/
│ └─foo.c
└─test/
  └─test_foo.c

bar/            # sources and tests for bar
├─Makefile
├─src/
│ └─bar.c
└─test/
  └─test_bar.c

Either way, there ought to be room in the project's directory structure for some sub-projects.

Filenames: I generally try to make one test per blah.c file and name it test_blah.c, but there's a catch. In C, mocking is about the only way to inject behavior. But you may want to have some test-cases where a particular function is mocked and others where it is not mocked. This absolutely necessitates different linkage. As a result, you need a different top-level executable. This means a production source might have multiple test-case files.

Rust quite elegantly solves the "where the heck is this test-case?" problem by putting tests in the same file as the sources. But the Rust build system is specifically designed for this.

I'd like to further propose that these test source files be located in /tests/cmocka/ or something similar.

Sure, we can do that, but I have some questions about layout.

Normally, a Makefile only builds code in its directory and lower. (Exceptions include vendored code and linking.) Right now, the Makefile in ARDOPC/ reaches out to ../ARDOPCommonCode/ and later it will do ../tests/cmocka as well. This is unusual.

Also, ARDOPCommonCode/ might be a misnomer now since it's where the main() routines and executable driver behavior lives, and it's not really "common" with anything anymore.

It would make more sense to adopt one of the above structures and possibly get rid of ARDOPCommonCode since it isn't common with anything anymore.

That said, I will definitely re-org this however you like.

What do you think of a recommendation to create new test cases for functions before rewriting them?

This is generally what I do, to the extent possible.

On this PR, I introduce some failing tests and then fix them with my changes to production code. Those changes are split into their own commits for easier review.

Most small projects don't/won't/shouldn't merge with failing tests, so the necessary fixes need to be made before a PR can be merged.

[pflarue]

Regarding layout and naming:

When I first created a fork of ardopc, my goal was to make it usable for myself with a few minor changes, and to offer those changes to others having similar problems. I expected changes that proved useful would be adopted by ardopc. As I made more changes to ardopcf, this became less likely. However, I still thought that even if new features added to ardopcf were unlikely to be included in ardopc, that selected bug fixes might be. With the recent cleanup work including whitespace changes, even this seems unlikely. My assumption now is that if anyone wants to adopt changes from ardopcf into ardopc or another fork of it, that they will probably do so by a manual copy and paste process. With this change in my assumptions, my earlier reluctance to change the layout of this project is gone. Thus, continued separation of files into the ARDOPC and ARDOPCommonCode directories is no longer appropriate.

@cbs228, I appreciate your willingness to suggest ways that we can improve the way we build and test ardopcf. I welcome these suggestions because I recognize that your knowledge and experience relating to these topics exceeds my own. I also hope that you will continue to offer such suggestions as we continue with this project. So, I would welcome a pull request from you to update the layout of this project. This could be done either as a new pull request, or as continued changes to this one, which ever you prefer.

I am curious what your recommendation are for where the contents of the lib directory should be placed in a revised layout. In case it influences your opinion on this topic, I wrote the contents of lib/ws_server, and contributed a good bit of code to an older library to produce the contents of lib/rockliff. While these two currently exist only here in this ardop repository, I plan to create independent repositories for each of them since I think that they may have value beyond ardop. I have not done so yet because ardopcf provides a useful opportunity to refine and debug them first. This plan, was why I initially placed both of these in the lib directory.

If you have suggestions to change the layout and handling of the html and js files for the webgui, I would also welcome those. In support of that possibility, I'll briefly explain my motivation for what is currently there. I really like that ardopcf can be distributed as a single executable file that can be run with no installation process. Thus, I wanted to include the html and js code within the compiled binary. The only possible downside I can see to this is that it increases the size of the binaries, but this does not seem significant for these small executable files. I also wanted to avoid over-complicating the build process to do this, and to ensure that whatever was done was portable to work equally well for both Linux and Windows. To facilitate debugging the js code in a browser, I also wanted to retain the structure of the included js code rather than using a minify process to produce js code that is more compact but no longer easily understandable. I'm happy to consider alternative solutions that still achieve these goals.

Of the layout examples you gave in one of your earlier comments in this pull request, I personally like the one with top level src and test directories. My reasoning for this is that, while I agree that testing is important, I think that keeping the test related code segregated from the main source code makes the main source code easier for someone looking at the project for the first time to study and understand.

[pflarue]

Earlier I reported that I was unable to get cmocka to build using winlibs 32-bit MinGW for Windows. Planning to try to identify and resolve the problems, I repeated the process. This time everything worked. My best guess is that I mistyped something when it failed. So, the following simplified comments can be included in Makefile:

#       In the following description of how to install cmocka for Windows, a
#       winlibs MinGW installation is assumed to be located at `C:\winlibs`
#       If installed elsewhere, substitute the appropriate path.  Putting the 
#       cmocka files into the winlibs install directory avoids the need for further 
#       configuration.  This uses git (available from https://git-scm.com/downloads/win) 
#       to download the cmocka source code. 
#
#       git clone https://git.cryptomilk.org/projects/cmocka.git
#       cd cmocka
#       mkdir build
#       cd build
#       cmake -G "MinGW Makefiles" -DCMAKE_INSTALL_PREFIX="C:\winlibs" ..
#       mingw32-make
#       mingw32-make install

[cbs228]

I am curious what your recommendation are for where the contents of the lib directory should be placed in a revised layout

I think those files can stay where they are for now. The third-party projects should stay together in case we need to exempt them from rules (like whitespace enforcement) and to indicate that they may have different licenses.

I do not recommend spinning any of those directories off into their own repository until there is actual demand for that. If there isn't, it just makes your work much harder for no reason. If you do spin them off, please consider using git-subtrees instead of the unending nightmare that is git-submodules.

Since these projects may have a life of their own, you may eventually want them to have their own src/ and test/ directories. This will ensure they are buildable and testable as their own targets. For the sake of parallel structure, this might indicate the same for ARDOPC.

If you have suggestions to change the layout and handling of the html and js files for the webgui

I haven't really looked at these yet, but I like just packaging the web files in with the executable.

Browsers support a source map so that your browser can derive and show you the unminified / unmangled source code in the web developer tools automatically. If you use this, you can have the best of both worlds. But since you are not serving the javascript over the internet, I wouldn't bother minifying it at all.

Let me put together a basic reorg and open a new PR for it. It is much easier to discuss and review code that actually exists.

[pflarue]

@cbs228, I'd like to discuss some details of this PR off-line. I sent an email to an address I found for you at the ardop users group, but don't know for sure whether that is a current address.

[pflarue]

After we get this merged, I'll take a closer look at the logging libraries you mentioned. An alternative is to move most of the the existing platform specific logging code into one of the files in the common directory. I think that only the functions used to generate timestamps (and thus log filenames) are actually platform specific.

Regarding use of a new structure to hold a callsign (local or remote), this is worth exploring further. However, it seems to me that it might make more sense to have two fields: a string for display purposes, and a byte array for use in control frames. Two constructors would be required, depending on which format is provided. Most callsigns (local and remote) would be constructed from strings, but upon receiving a ConReq, IDFrame, Ping, etc, the callsign from the received frame would be constructed from a byte array. Except for Compressing/DeCompressing, the values of the callsign w/o SSID and of the SSID are never explicitly used. Given the string representation as one of the fields, its length can always be determined with strlen(), so I don't see a need to provide a field for this value.