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

Good news everyone: I've refactored the tests again! #7

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

Conversation

henryefranks
Copy link
Collaborator

I'd like to begin by thanking the various coffee shops of Cambridge, particularly their free Wi-Fi, for their contribution towards this pull request.

So I've refactored the tests again. What's changed this time?

The biggest new features are automatic test discovery and less dependence on environment variables. Having said that, I haven't been able to eliminate COCOTB_TOP and RTL_TOP entirely. Let's start with the good news: the call syntax for the cocotb_test_wrapped has been significantly streamlined to make writing tests easier. Take a test case for my decoder as an example:

src_dirs = [
    'decode/henry_decoder'
]

def test_priority_encoder():
    cocotb_test_wrapper(
        src=src_dirs,
        toplevel='count_regime_16',
        test_search_path='priority_encoder'
    )

We can restrict the source files globbed by the compiler with the src argument, used here to stop compilation errors with unrelated code from failing this test suite (hint: this is generally a good idea if you can make it work with your tests). src_dirs is defined outside the function scope because all the tests in this file use the same source files. Not passing an argument here compiles the whole hdl/rtl/src directory. In addition, you can provide a custom include path with the include argument; by default, this points to hdl/rtl/include, where I've placed some of our existing files as new header files.

The toplevel arg is needed to tell both the compiler and runner which module is our top. The duplication there annoys
me and makes my original plan of sharing a single object between all tests in a module impossible (for now). I'd quite like to pursue this in future because we're compiling the same code many times across any given test suite, so I'll investigate if something like '-' or specifying multiple top level modules could fix this issue.

test_search_path is new, and, uses glob to recursively find all files named cocotb_test_*.py from within the specified directory. This allows us to write multi-file test suites for modules. However, we already have a lot of flexibility across multiple pytests within multiple test_*.py files, so we should review this approach.

I've added some other args to control flags etc, but I doubt we'll be using them much

I've hacked this all together to work more robustly with the GitHub Actions scripts too, so that's all looking good.

Unfortunately, there are still a few drawbacks to this approach:

  • Individual source files can't be included, only directories to glob.
  • Tests still need to be written in this awkward syntax. I've looked into some pytest parameterisation techniques but they're messy and don't work well with the xdist multithreading
  • We're still compiling the same code a bunch of times with slightly different top levels. if we can fix this, I'd love to.
  • The test structure still feels like it has one or two degrees of freedom too many.
  • We need to add timeunit/timeprecision control back at some point (this wasn't in the previous code either).
  • We still need to run the export RTL_TOP= etc commands to get the right paths for compilation (RTL_TOP) and saving the results (COCOTB_TOP). I've got rid of the GitHub script and just moved the commands into verif.yml, which works well enough. The benefit of this approach is that we can specify entirely relative paths, and only have to worry about doing any path logic in the coco_wrapper module.
  • If possible, it would be good to separate the build and results directory. This approach would be a good step towards my eventual goal of sharing the compiled code between all tests in a suite, and would let us share .o files while keeping results entirely separate. Even named results files would be enough for this, but it'll require some looking into at some point down the line.

Before we merge this PR, let's make sure all these improvements are copied to issues or that project board so I remember to actually make the changes. Same with the last PR.

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.

1 participant