Add test to ensure PERFETTO is disabled by default

[sudara]

Since I toggle the default sometimes locally, it would be good to have a sanity check that tests PERFETTO=0

[benthevining]

Hmm... As of now, the PERFETTO toggle is exposed as a CMake option. I understand wanting to check that it's off by default, but if someone builds and runs the tests with:

cmake -B build -D PERFETTO=ON
cmake --build build
cd build && ctest

That is a perfectly valid usage, and would cause this test case to fail.

Perhaps what we should really test is that this line doesn't get changed to define it to 1 by default. We could do this with some sort of regex-based script that gets executed as a test case?

[sudara]

That is a perfectly valid usage, and would cause this test case to fail.

Hm, is the issue that we're talking about a Catch2 test, so it would be incompatible with other tests that set PERFETTO=1? Could we check PERFETTO=0 on a cmake test that does a default build and call it a day?

[benthevining]

That's a good point, we can't have some test cases that require PERFETTO=1 and others that require PERFETTO=0. The issue here is that with the current workflow, PERFETTO is a build-wide setting, so it will either be on or off for all test cases.

At the CMake level, we can do something like

if(PERFETTO)
  add_test (test_case_that_checks_perfetto_stuff)
else()
  add_test (check_that_perfetto_is_off)
endif()

but that approach will break down if you manually change the define in the C++ source code.

[benthevining]

Could we check PERFETTO=0 on a cmake test that does a default build and call it a day?

Yes, we could. The caveat being that if someone happens to run the tests with the PERFETTO setting on, then that test case would fail (expectedly).

[sudara]

if someone happens to run the tests with the PERFETTO setting on, then that test case would fail (expectedly).

Hmm, I actually have to admit to 0 knowledge of how the cmake tests are working. I thought they were sort of isolated / independent and so could have different settings, etc. If that's true, I think it's a good thing that the test would fail with the PERFETTO setting enabled...

But yeah, for Catch2, seems like we'd just have to pick one way, which I guess is mostly PEFETTO enabled?

Hm or maybe we can run 2 Catch2 targets, one with it off, one with on.

[benthevining]

I thought they were sort of isolated / independent and so could have different settings, etc.

It is true that different CMake targets can have different compile definitions. But the way we're doing it now is that there is also a CMake option called PERFETTO, which controls whether the PERFETTO define is 1 or 0 for all targets globally in the entire build. Perhaps this was a bad design decision?

[sudara]

Ah, I see — so you are saying by specifying an option, it won't really matter what's in the C++ file? But i guess we could have a CMake test that doesn't specify an option, confirms it's the default 0?

[benthevining]

Here's how it currently works:

In the C++ file, we have:

#ifndef PERFETTO
  #define PERFETTO 0
#endif

so the PERFETTO symbol will be 0 by default if it wasn't explicitly set to 1.

I thought there should be a way to specify from the CMake command line whether that PERFETTO symbol should be on or off. You can't directly specify compile definitions on the CMake command line, so I created a CMake variable that can be set, which the scripts will then act on to pass the appropriate definitions on to the compiler.

In the CMake code, we've currently got this:

option(PERFETTO "Enable Perfetto tracing using the melatonin_perfetto module" OFF)

if (PERFETTO)
    target_compile_definitions(melatonin_perfetto INTERFACE PERFETTO=1)
endif ()

What this means is:

• There's a CMake cache variable, also called PERFETTO, that defaults to OFF unless the user explicitly turns it on on the command line (with -D PERFETTO=ON)
• If that PERFETTO variable is on, then we tell CMake to define the PERFETTO symbol to 1 for the compiler. The INTERFACE keyword here means that this definition will be passed on to all targets that link against melatonin_perfetto

So, with this current approach, if someone links their targets against melatonin_perfetto and specifies nothing else, the PERFETTO symbol will end up being 0 in all of their targets. If they run cmake with something like

cmake -B build -D PERFETTO=ON

then the PERFETTO symbol will end up being 1 in all of their targets.

I think this approach is desirable, because it easily allows the developer to turn profiling on and off from the command line without changing any of their C++ or CMake code. However, the tradeoff here is that the PERFETTO symbol will have the same value for every target in the build (because the melatonin_perfetto JUCE module target is specifying that symbol's value, and is passing it on to everything that links against it).

I don't think this is a huge drawback, except for our internal tests, because as we've discovered, some of them need PERFETTO to be defined to 1, and some of them need PERFETTO to be defined to 0.

There are several possible solutions:

• Within the CMake code, selectively add test cases depending on whether the PERFETTO CMake cache variable is ON or OFF.
• Override the PERFETTO symbol in certain test targets. This could be done in the C++ code such as:

  // in test.cpp
  #undef PERFETTO
  #define PERFETTO 1
  #include <melatonin_perfetto/melatonin_perfetto.h>

• Get rid of the global PERFETTO CMake variable. If we go this route, then users would need to manually do something like

  target_compile_definitions(TheirTarget PRIVATE PERFETTO=1)

  for each target they want to enable profiling on. I think this reduces convenience, but perhaps allowing more granularity and not requiring profiling to be globally enabled/disabled for all targets is desirable.

[sudara]

Thanks for laying it all out! Ok, I'm on the same page.

We should definitely keep the option. I also think it's convenient. (It making testing hard isn't a good reason to remove it)

Override the PERFETTO symbol in certain test targets.

The only disadvantage here is we also want to test the out of the box default behavior of the module, and make sure it does the right thing on app quit, etc (aka, not dump a trace).

What do you think about having a test suite for ON and a test suite for Default? We can have 2 test targets that pluck their tests from 2 different folders?

[sudara]

will have the same value for every target in the build

Ah ok, this clicked a bit here — it's build level, not test level.

I guess we could matrix the github action? Or run both serially? There's no reason to optimize for anything other than doing exactly what we need on GA, I think...

[benthevining]

it's build level, not test level

Yes exactly.

I think running both serially or matrixing the GHA script is the best solution here.

Within the CMake code, I think we should selectively add test cases depending on the value of the PERFETTO CMake cache variable, such as:

if(PERFETTO)
  # add test cases that require `PERFETTO=1`
else()
  # add test cases that require `PERFETTO=0`
endif()

And then we can run the build & tests twice, once with PERFETTO=OFF and once with PERFETTO=ON.

[sudara]

Sounds great!

once with PERFETTO=OFF and once with PERFETTO=ON.

What do you think about the OFF run actually being a default run, with no option specified?

[benthevining]

Yes, that sounds perfect to me. In fact, we could even:

• First run with nothing specified, test that the default is profiling off
• Then re-run cmake with -D PERFETTO=ON, test that profiling is now on
• Run again with -D PERFETTO=OFF, test that profiling is correctly turned off

though that may be overkill

[sudara]

I'll let you decide! I'm mainly just happy to get that default tested! (and be able to do things like making sure the default doesn't drop a trace file, etc....)

[benthevining]

Totally. I'll work on adding these test cases.

One thing that would simplify writing the test case that checks the correct dump file is created would be if there was an API to get the dump file directory, something like:

class MelatoninPerfetto
{
  static juce::File getDumpFileDirectory();
};

If you have no objections, I'll create a PR for this.

[benthevining]

Actually, what about having the endSession method return a juce::File object that represents the dump file that was written to? 🤔

[sudara]

Both of those suggestions sound good to me! writeFile is a bit of a clunky method, would make sense to remove the file name/dir logic from it.

[benthevining]

Awesome. I've implemented this on the more-tests branch, I'll work on adding the test case to check that the file is actually created.

[benthevining]

On the more-tests branch, there's now a RunTests.py script that runs CMake build, configure, and test multiple times in sequence.

I've verified that changing either the default value of the PERFETTO CMake option on this line or the default value of the PERFETTO preprocessor symbol on this line will cause test failures.

[sudara]

Amazing! Thank you! I'll open a PR.