Adding gmock to test dependencies

Googlemock is a C++ testing library that is tightly coupled with Googletest. It augments gtest with a large suite of useful matchers; most of Matchers Reference | GoogleTest comes from gmock.

I’d like to add it to .github/actions/setup-build-env/action.yml to give us more robust tests with expressive errors.

I also noticed that the above file isn’t fully configured correctly to install new packages in the CI’s image. A -y needs to be added to the apt-get install to actually install the packages. It seems like all of the packages listed there today are already present in the default CI image?

This also exposed that .github/workflows/test-verific.yml is not running the same build environment as the other CI flows. We should add a new step that sources the above setup-build-env/action.yml to bring it to parity with the other flows.

These changes are already included in Logging: Add log sink system to replace ostreams by Logikable · Pull Request #5311 · YosysHQ/yosys · GitHub , but I’ll move them out to their own PR as requested once this suggestion is approved.

I also noticed that the above file isn’t fully configured correctly to install new packages in the CI’s image. A -y needs to be added to the apt-get install to actually install the packages. It seems like all of the packages listed there today are already present in the default CI image?

I don’t know exactly how it works, but the github runners are installing packages just fine without -y. I’m not sure if the action logs are publicly accessible or not, but at a brief glance:

Run sudo apt-get update
  sudo apt-get update
  sudo apt-get install gperf build-essential bison flex libfl-dev libreadline-dev gawk tcl-dev libffi-dev git graphviz xdot pkg-config python3 libboost-system-dev libboost-python-dev libboost-filesystem-dev zlib1g-dev libbz2-dev libgtest-dev
...
Get:40 http://azure.archive.ubuntu.com/ubuntu noble/universe amd64 libgtest-dev amd64 1.14.0-1 [268 kB]
...
Selecting previously unselected package libgtest-dev:amd64.
Preparing to unpack .../38-libgtest-dev_1.14.0-1_amd64.deb ...
Unpacking libgtest-dev:amd64 (1.14.0-1) ...

The runner images are also documented, and there is no gtest listed there, so that package install is coming from the apt-get install.

IIRC, using the private runner (the one used in test-verific) does require the -y, but as you discovered that runner is using a pre-configured Docker image rather than installing the necessary packages. It is also specifically not kept in parity with the other flows because there are additional (typically longer running) tests that are only run on the private runner, meaning it has extra requirements which are not necessary for the standard test suite on the public runners. I believe this is a super-set of the standard requirements, so in principle there shouldn’t be anything wrong with changing it to use the same setup action (and indeed looking at #5311 that seems to have worked fine).

As I understand it, we do have some unit tests that use gtest, but there are not many and adding more is very much welcome. Adding gmock as an additional requirement to running those tests should be fine. Though I do think we should make an effort to separate test-dependencies from build-dependencies for clarity and simplicity when it comes to maintaining the documentation on installation instructions. The prerequisites in the documentation and in the action should be the same, though that is not quite true at the moment (ideally both would be able to include a single file rather than maintaining the same list twice).

If you’re familiar with gtest, it would be great if you could take a look at the test suite docs. There is a bunch of commented out stuff that I have no idea about how accurate it is. Even if you don’t do anything with the documentation that is there, it would be great to at least include the additional requirements needed for running make unit-test if you’re going to be adding an extra one (doing it in the same PR as adding gmock is fine).

Also worth noting that the apt-get is only used for Linux runners. test-sanitizers doesn’t run on PRs (due to runtime concerns), but that will trigger the unit tests running on a macOS runner, which I suspect will fail with your current code changes. The job is available to be run on-demand, which you can either do from a fork or ask a maintainer to do once you’ve made a PR. Probably worth giving that a run before merging to avoid CI failing when it tries to run the sanitizer tests on main.

Thank you! I’ll look into excluding the setup / having a different setup for the macOS runner, and update the test suite docs.

FYI I made a PR which shuffles around some of the prerequisites and adds some basic information on the test suites doc page based on my last message.