Add test suite for `icemulti'

[rlutz]

This pull request adds the target check as a place to add tests for the individual tools. It calls the existing icebram test, but as this depends on additional tools being installed (Yosys, arachne-pnr, IVerilog), it is skipped if these tools aren't found in PATH. It then adds a test suite for icemulti which covers every possible combination of input files as well as typical valid and invalid values for the command-line options.

[cliffordwolf]

Why do we need >200kB of .hex files? Wouldn't checksums suffice?

I think the "skipping" stuff in icebram Makefile is horrible: If you run "make check" and you don't have the tools installed then it should simply fail imo.

Historically something like that is called "make test" in icestorm (see Makefiles in icecompr, icebram, icetime). If you want to rename this from "test" to "check", then we can discuss this: Why do you want to do this?

However, the "test check:" target you added in icebram/Makefile is imo bogus. Either we call it "test" or we call it "check". But I don't see anything to gain by supporting both names in some subdirs (but not in others?)..

Also: icetime already has a "make test" but you added an empty "make check" target. Why?

@martinda

[rlutz]

Why do we need >200kB of .hex files? Wouldn't checksums suffice?

They would suffice for detecting if there are differences, but in order to debug the situation, we would want to know what the differences are. That's why the correct output is included in the test suite.

The rationale for using hex files as opposed to binary files is that they make more sense in terms of source code management. There are some commits which change the output icemulti generates; this way, it is easy to see from a diff how the output changes.

I think the "skipping" stuff in icebram Makefile is horrible: If you run "make check" and you don't have the tools installed then it should simply fail imo.

I think the default test suite for a package should always be able to run without requiring the user to install any addtional software.

If you want the icebram test to be able to run without additional tools, you could include the pre-generated input files and the correct output in the repository and have the check target only execute the line

./icebram -v demo_dat0.hex demo_dat1.hex < demo.asc > demo_new.asc

This way, any unwanted changes in icebram behavior could be detected while the complete demo could still be run manually.

Historically something like that is called "make test" in icestorm (see Makefiles in icecompr, icebram, icetime). If you want to rename this from "test" to "check", then we can discuss this: Why do you want to do this?

As far as I am aware of, check is the usual target for running a test suite. This is, for example, reflected in the GNU Coding Standards: https://www.gnu.org/prep/standards/standards.html#Standard-Targets

However, the "test check:" target you added in icebram/Makefile is imo bogus. Either we call it "test" or we call it "check". But I don't see anything to gain by supporting both names in some subdirs (but not in others?)..

I didn't see a reason to remove the existing target test; but you're right, when introducing a global target check, it's probably best to just rename that.

Also: icetime already has a "make test" but you added an empty "make check" target. Why?

Oops, I missed that. However, the same problem as with the icebram demo applies here, too—even in a more pronounced way as the user would have to license the proprietary iCEcube2 software in order to run the icetime checks. Maybe it's possible to separate the icetime tests from the other actions of the script; I haven't looked at it in detail yet.

[cliffordwolf]

I think the default test suite for a package should always be able to run without requiring the user to install any addtional software.

What?!

So no test suite must use a unit testing framework for example, unless the same unit testing framework is also required to build the software? Sorry, but that is obviously a nonsensical requirement.

[rlutz]

So no test suite must use a unit testing framework for example, unless the same unit testing framework is also required to build the software?

I don't know which test frameworks you are talking about, but the ones I've been using so far (Autotest as well as Automake's serial and parallel test harnesses) go to great lengths to ensure everything the user needs in order to run the test suite is included in the distribution tarball.

The way I see it, a test suite's worth is in people running it. Running the tests should be a natural part of committing some changes, preparing a distribution, or installing a software package, so the traditional

./configure && make && make install

gradually evolves to become a

./configure && make && make check && make install

But people won't run tests if they are slow or if they can't run them. The idea that people stop to install extra software on their machine is illusive; instead, they just won't run the tests.

Requiring Yosys and arachne-pnr in an IceStorm test is probably okay since in the most common use case, they are part of the same toolchain. However, having the toplevel make check fail unless a proprietary software suite has been installed is IMHO counterproductive.

[cliffordwolf]

I don't know which test frameworks you are talking about, but [..]

To name just one: gtest

[rlutz]

I changed the pull request according to your feedback.

[martinda]

@rlutz, I never saw the : being used this way :;, what is the purpose?

[cliffordwolf]

@martinda The purpose is apparently to force "make" to run the command in a shell (which it for example does when the command string contains a semicolon), which is necessary because type is a shell builtin. My two issues with this are:

(1) I just makes thing more complicated than they need to be. For example, the command checking icecube2 in the icetime makefile just replicates the icecube version number and if someone updates the icecube version used in icefuzz then it is very likely that we will forget to update the version number in that Makefile. And adding this command it gains us nothing! The icecube.sh shell script is executed with set -ex, so if the icecube synthesis command is not present we will see exactly which command failed and why. Adding this additional check is just unnecessarily overcomplicating things.

(2) The :; hack is ugly. If you want to explicitly call something using the shell, then simply explicitly call the shell: bash -c "type /opt/lscc/iCEcube2.2015.08/LSE/bin/lin/synthesis". But as I said in (1), I don't see the need to do anything like that at all!

[rlutz]

I removed the global target check and the checks for required tools (i.e., anything but the test suite itself) from the pull request. If there's anything else you want me to change, please let me know.

[rlutz]

I'm not sure if that's what kept you from merging the pull request, but on request of @martinda, I removed the reference files, too.

[rlutz]

The problem with the test suite as it is now is that it makes it hard to verify whether the output of new or changed tests is valid before merging them (which I did for the tests I submitted, and without which they are kind of pointless).