Unit testing #2536

crsib · 2022-02-08T18:42:12Z

Resolves: #2478

This PR introduces a framework for automated testing in Audacity. It is now possible to define the following types of tests:

Unit Tests

add_unit_test(NAME name SOURCES file1 ... LIBRARIES lib1 ...)

This function creates an executable called ${name}-test from the sources files (${file1}, ...). and linked to libraries (${lib1}, ...).

Catch2 is linked implicitly. There is no need to define a main function in the unit test.

Journal Tests

add_journal_test(journal_file)

Adds a test, that runs Audacity with the journal ${journal_file}. The test name is based on the name component of the ${journal_file}.

Both unit and journal tests are registered with the CTest framework. This PR implements unit tests for FromChars set of functions and a journal test that checks that we can run Audacity at all.

To be able to run journal tests Audacity now suppresses a few modal dialogs, shown during the startup:

update permission dialog,
auto-recovery dialog,
extension registration on Windows.

For libraries, if the libraries/(name)/tests directory is present - CMake will include the unit tests for the library automatically.

The Build Audacity workflow will run both unit and journal tests for each build. In the future, journal tests can be separated into a different nightly workflow.

Examples

Adding a unit test:

add_unit_test(
   NAME
      lib-string-utils
   SOURCES
      FromCharsTests.cpp
   LIBRARIES
      lib-string-utils
)

Adding a journal test:

add_journal_test( "journal_sanity.txt" )

Running tests:

$ ctest -C Debug

I signed CLA
The title of the pull request describes an issue it addresses
If changes are extensive, then there is a sequence of easily reviewable commits
Each commit's message describes its purpose and effects
There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

Each commit compiles and runs on my machine without known undesirable changes of behavior

libraries/lib-string-utils/tests/FromCharsTests.cpp

Paul-Licameli · 2022-02-09T02:48:23Z

Wow! This is wonderful. Thanks for figuring all of this out.

I have not approved all yet -- I want to read and understand all of AudacityTesting.cmake.

Very likely those CMake functions will need enhancement with more options. Journal tests may need to run with a certain prior environment setup, such as having a particular audacity.cfg file already in place at startup, and then the environment would need cleanup after the test. CTest gives you the means to do such things by defining setup and teardown commands as "test" too in a certain dependency graph. We may also want to associate labels with test so we can filter them and run subsets.

But all that can be future work and no reason to delay this.

cmake-proxies/cmake-modules/AudacityTesting.cmake

Paul-Licameli · 2022-02-18T11:03:36Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+   #[[
+      add_unit_test(NAME name SOURCES file1 ... LIBRARIES lib1 ...)
+
+      Creates an executable called ${name}-test from the sources files ${file1}, ... and linked


"source files"

It may be useful to add PROPERTIES prop1 ... and pass those to: https://cmake.org/cmake/help/latest/command/set_tests_properties.html?highlight=set_tests_properties

And here are all the possible properties https://cmake.org/cmake/help/latest/manual/cmake-properties.7.html#test-properties

Maybe the function will have other keyword arguments to abbreviate other combinations of properties. Don't ask me yet, what exactly. Experience will tell what is useful.

This can be easily added in the future in case we find a need for any additional ctest properties

Paul-Licameli · 2022-02-18T11:10:45Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+      Creates an executable called ${name}-test from the sources files ${file1}, ... and linked
+      to libraries ${lib1}, ... Catch2 is linked implicitly.
+
+      Create a CTest test called ${name}, that expects 0 code on success


... unless one uses one of the properties:

FAIL_REGULAR_EXPRESSION

PASS_REGULAR_EXPRESSION

SKIP_REGULAR_EXPRESSION

SKIP_RETURN_CODE

WILL_FAIL

I have a very strong opinion that unit test should return 0 on success

Paul-Licameli · 2022-02-18T11:14:21Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+      # Create test executable
+
+      add_executable( ${test_executable_name} ${ADD_UNIT_TEST_SOURCES} "${CMAKE_SOURCE_DIR}/tests/Catch2Main.cpp")
+      target_link_libraries( ${test_executable_name} ${ADD_UNIT_TEST_LIBRARIES} Catch2::Catch2 )


Catch2 needs to link something? I thought it is just one amalgamated header file. Or is that precisely what this line does, just adding in include path.

"Modern" CMake uses target_link_libraries for header-only libraries as well.

cmake-proxies/cmake-modules/AudacityTesting.cmake

Paul-Licameli · 2022-02-18T11:19:47Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+         NAME
+            ${ADD_UNIT_TEST_NAME}
+         COMMAND
+            ${test_executable_name}


Will we want arguments for COMMAND that can be given to add_unit_test?

Will we ever need to reuse a common test executable with different arguments?

Should there be a lower-level CMake function taken out of this, for invoking add_test with such options?

I'm thinking we might in future need a more complicated thing like setup and cleanup of fixtures for a test or a group of tests, in which each setup or cleanup step is itself described as a "test." https://cmake.org/cmake/help/latest/prop_test/FIXTURES_REQUIRED.html#prop_test:FIXTURES_REQUIRED

And still, I have very that if you need to it for unit tests you have serious design issues and should reconsider them. add_unit_test is expected to be used only to create unit tests.

Paul-Licameli · 2022-02-18T11:23:06Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+            ${ADD_UNIT_TEST_NAME}
+            PROPERTIES
+               ENVIRONMENT "DYLD_FALLBACK_LIBRARY_PATH=$<SHELL_PATH:${_SHARED_PROXY_BASE_PATH}/$<CONFIG>>"
+         )


I wonder if these fixes you figured out for dynamic libraries should be reused with image-compiler too. Or have you done that already?

Maybe a common cmake function for such steps then.

They can be reused, but the image-compiler is a) rarely built, b) does not test the functionality of a library. For these reasons, it is left as-is for now.

Paul-Licameli · 2022-02-18T11:25:55Z

cmake-proxies/cmake-modules/AudacityTesting.cmake

+            ${test_name}
+         COMMAND
+            ${audacity_target} --journal ${journal_file}
+      )


Again maybe use a common low function for add_test, and consider how to pass other command arguments and test options into it.

Paul-Licameli

I have finished my review.

Just some spelling mistakes in comments in cmake scripts that should be fixed immediately. Otherwise, suggestions for improvements, but nothing that needs to be done before merging this.

Paul-Licameli · 2022-02-18T11:32:28Z

I'm trying it in the Xcode build but I don't yet see how to build the tests.

Wait, there is a configuration error

Paul-Licameli · 2022-02-18T11:34:13Z

I couldn't build the tests because reconfiguring at the command line failed:

CMake Error at cmake-proxies/cmake-modules/AudacityFunctions.cmake:524 (add_subdirectory):
  The source directory

    /Users/paullicameli/Github/audacity/libraries/lib-strings/tests

  does not contain a CMakeLists.txt file.
Call Stack (most recent call first):
  cmake-proxies/cmake-modules/AudacityFunctions.cmake:563 (audacity_module_fn)
  libraries/lib-strings/CMakeLists.txt:33 (audacity_library)

Journal does not handle the case, when a modal dialog is displayed during the startup. In this case `CM,Exit` will fail to close Audacity.

Fixes typos

Paul-Licameli · 2022-02-18T11:37:08Z

Oh forget that! The directory tests was a bit of junk just on my own file system, from my own experiements with tests.

crsib · 2022-02-18T11:37:31Z

This looks to be your local issue, lib-strings doesn't have tests directory.

I think this is a leftover from the time you explored CTest

Paul-Licameli

Reapproval

Paul-Licameli · 2022-02-18T11:38:32Z

OK now I see tests in the Xcode browser. Waiting to build and run them.

Paul-Licameli · 2022-02-18T11:39:16Z

There are all those CI build failures however

crsib · 2022-02-18T11:39:58Z

Gosh Python world is sad :-(

aws/aws-sam-cli#3661

Paul-Licameli · 2022-02-18T11:40:39Z

"Error: The process '/Users/runner/hostedtoolcache/Python/3.10.2/x64/bin/conan' failed with exit code 1"

I was glancing quickly at that, and misread the word "headache" in there

crsib · 2022-02-18T11:41:54Z

Some third-party library broke half of the world again...
And I had issues with it when I was doing offline Linux builds as well. It is simply improperly packaged.

Paul-Licameli · 2022-02-18T11:46:03Z

I have built the tests in XCode, and confirmed that build fails if I edit the test program to have a syntax error.

Now I added REQUIRE(false) and I want to observe the consequences of a test failure. How?

Paul-Licameli · 2022-02-18T11:48:59Z

Aha, I see RUN_TESTS

Great, I see test outputs. Now we might consider what other CTest options or stderr messages from Audacity might make this display more understandable.

crsib · 2022-02-18T11:49:39Z

There are different ways:

You can select a specific test as a run target on XCode (if you want to debug the failure)
You can build RUN_TESTS

Paul-Licameli · 2022-02-18T11:51:43Z

The journal sanity test makes a lot of spam, and so will every journal test:

2: 06:47:42: Debug: Warning: command doesn't exist: 'Cut'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Copy'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Paste'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Trim'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Silence'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Undo'
2: 06:47:42: Debug: Warning: command doesn't exist: 'Redo'
2: 06:47:42: Debug: Warning: command doesn't exist: 'ZoomIn'
2: 06:47:42: Debug: Warning: command doesn't exist: 'ZoomOut'
2: 06:47:42: Debug: Warning: command doesn't exist: 'ZoomToggle'
2: 06:47:42: Debug: Warning: command doesn't exist: 'ZoomSel'
2: 06:47:42: Debug: Warning: command doesn't exist: 'FitInWindow'
2: 06:47:42: Debug: Unrecognized accel key 'NUMPAD_ENTER', accel string ignored.

Let's have an issue to clean that up

Paul-Licameli · 2022-02-18T11:57:16Z

And still, I have very that if you need to it for unit tests you have serious design issues and should reconsider them. add_unit_test is expected to be used only to create unit tests.

For unit tests, avoid fixtures for sure, but for the high level journal tests, fixtures could make sense.

Paul-Licameli · 2022-02-18T12:00:44Z

I have a very strong opinion that unit test should return 0 on success

But again, consider journal tests. Maybe we will want those to write stuff to stderr to be filtered, and we will want to define success and failure some other way based on outputs. @Penikov might use capabilities like that.

crsib · 2022-02-18T12:03:46Z

Well, add_journal_test will be modified accordingly then :)

Like we can say add_journal_test( fail.txt EXPECT_FAIL ) or similar.

Paul-Licameli · 2022-02-18T12:05:28Z

There are different ways:

You can select a specific test as a run target on XCode (if you want to debug the failure)

You can build RUN_TESTS

Good, I have confirmed that I can put a break in a test executable and hit it in the debugger.

Paul-Licameli · 2022-02-18T12:38:36Z

🥳

crsib requested a review from Paul-Licameli February 8, 2022 18:42

crsib added this to In progress in Sprint 13 - Audacity 3.2 via automation Feb 8, 2022

crsib moved this from In progress to Review in progress in Sprint 13 - Audacity 3.2 Feb 8, 2022

Paul-Licameli reviewed Feb 9, 2022

View reviewed changes

libraries/lib-string-utils/tests/FromCharsTests.cpp Outdated Show resolved Hide resolved

Paul-Licameli reviewed Feb 9, 2022

View reviewed changes

libraries/lib-string-utils/tests/FromCharsTests.cpp Outdated Show resolved Hide resolved

crsib force-pushed the 2478_unit_testing branch from 39177e4 to 6d58de4 Compare February 9, 2022 09:11

Paul-Licameli reviewed Feb 18, 2022

View reviewed changes

cmake-proxies/cmake-modules/AudacityTesting.cmake Outdated Show resolved Hide resolved

Paul-Licameli reviewed Feb 18, 2022

View reviewed changes

cmake-proxies/cmake-modules/AudacityTesting.cmake Outdated Show resolved Hide resolved

Paul-Licameli reviewed Feb 18, 2022

View reviewed changes

Paul-Licameli previously approved these changes Feb 18, 2022

View reviewed changes

Sprint 13 - Audacity 3.2 automation moved this from Review in progress to Reviewer approved Feb 18, 2022

Sprint 13 - Audacity 3.2 automation moved this from Reviewer approved to Review in progress Feb 18, 2022

crsib added 5 commits February 18, 2022 14:35

Suppress startup modal dialogs when playing back the Journal

5978712

Journal does not handle the case, when a modal dialog is displayed during the startup. In this case `CM,Exit` will fail to close Audacity.

Adds a framework to simplify tests integration into Audacity

75f4df5

Fixes typos

Adds unit tests for the FromChars

a93edc0

Adds a journal based test to check that Audacity actually runs

e7def86

Adds automated tests to the CI workflow

ef3921d

crsib force-pushed the 2478_unit_testing branch from 6d58de4 to ef3921d Compare February 18, 2022 11:35

Sprint 13 - Audacity 3.2 automation moved this from Review in progress to Reviewer approved Feb 18, 2022

Paul-Licameli approved these changes Feb 18, 2022

View reviewed changes

Paul-Licameli mentioned this pull request Feb 18, 2022

Understandable journal test results: Eliminate Audacity's spam on stdout and stderr #2569

Closed

crsib merged commit 1ed3a29 into audacity:master Feb 18, 2022

Sprint 13 - Audacity 3.2 automation moved this from Reviewer approved to Ready for QA Feb 18, 2022

crsib deleted the 2478_unit_testing branch February 18, 2022 12:27

Penikov moved this from Ready for QA to In QA in Sprint 13 - Audacity 3.2 Feb 21, 2022

Penikov moved this from In QA to Done in Sprint 13 - Audacity 3.2 Feb 22, 2022

Unit testing #2536

Unit testing #2536

Conversation

crsib commented Feb 8, 2022

Unit Tests

Journal Tests

Examples

Paul-Licameli commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Licameli Feb 18, 2022 • edited

Choose a reason for hiding this comment

Paul-Licameli left a comment

Choose a reason for hiding this comment

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

crsib commented Feb 18, 2022 • edited

Paul-Licameli left a comment

Choose a reason for hiding this comment

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

crsib commented Feb 18, 2022 • edited

Paul-Licameli commented Feb 18, 2022

crsib commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

crsib commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

crsib commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli commented Feb 18, 2022

Paul-Licameli Feb 18, 2022 •

edited

crsib commented Feb 18, 2022 •

edited

crsib commented Feb 18, 2022 •

edited