Skip to content

Require test scripts for (most) CW codes in LALApps

Description

This MR is part of a series of changes with the eventual goal of moving most CW codes (by which I mean executable programs) from LALApps to LALPulsar.

As part of that move, I want to enforce that all CW codes are executed as part of a test script. This has many advantages:

  • Ensuring as best we can that CW codes are not broken by trivial changes (e.g. fixing compiler warnings); see e.g. https://git.ligo.org/CW/software/lalsuite/-/issues/62
  • Giving confidence that changes such as !1728 (merged) are tested on all CW codes that are part of LALSuite
  • Show some examples of usage (in lieu of more detailed documentation) to show how CW codes are used
  • Generally ensuring that CW codes remain well-tested and unable

In future, we would only add new CW codes to LALPulsar, and require them to have test suites.

This MR includes changes that enforce that requirement. The test scripts run under lalapps/src/pulsar, which already used a custom "runner" script, will now log which executables under lalapps/src/pulsar are called from the test suite. Then, lalapps/src/pulsar/Makefile.am includes a rule to check for any executables under lalapps/src/pulsar that were not called from the test suite, and raises and error if any were found.

Given that the lalapps/src/pulsar test suite is incomplete (#327 (closed)) I have only fully enabled this feature for these subdirectories: FITSTools Fstatistic GCT MakeData SFTTools Tools Weave. For Fstatistic and Tools I've written some new tests, included in this MR, to complete the test suite in those directories.

For the other directories:

  • CreateEphemeris: currently no test suite, and I'm not qualified to write one; see #328 (closed)
  • CrossCorr: one test script only for lalapps_pulsar_crosscorr_v2, which runs the code but doesn't check its output. I've had to disable this test for now. See #494
  • Fscan: this directory contains a lot of scripts without tests. For now I've marked those scripts are being not-installed, so that they're not required to have tests. Longer terms, these scripts should probably be moved out of LALSuite.
  • HeterodyneSearch: these codes depend on LALInference and so can never be moved to LALPulsar. They might be moved to LALInference, or else remain in LALApps. See #320 (closed)
  • Hough: currently no test suite, and I'm not qualified to write one; see #329 (closed)
  • HoughFstat: currently no test suite and no maintainer
  • HWInjection: this code depend on FrameL directly, and so would have to stay in LALApps. I've also far from qualified to write a test suite. I've moved this code back to src/hwinjection instead of src/pulsar/HWInjection, reverting 709b4b40 and a2dec85f
  • SidebandSearch: currently no test suite and no maintainer
  • TwoSpect: current test script does nothing; see #330

I won't be pushing people to add test suites for these codes, and for that reason will close #327 (closed). But neither will I mind much if these codes end up in a broken state if there was no test suite to check them.

There are a few build system changes:

  • lalapps/gnuscripts/lalapps.am now includes extra code to run test scripts written in Python, and uses the test_programs, test_scripts, and test_helpers format copied from gnuscripts/lalsuite_test.am
  • lalapps/gnuscripts/lalapps_pulsar.am (was lalapps/gnuscripts/lalapps_pulsar_test.am) now includes the code from lalapps/gnuscripts/lalapps.am which links against liblalapps.la, as well as the TESTS_ENVIRONMENT variable. Essentially lalapps/gnuscripts/lalapps_pulsar.am is now a drop-in replacement for lalapps/gnuscripts/lalapps.am, with the addition CW test suite specific code. This separation will make it easier to move CW codes into LALPulsar eventually.

Closes #327 (closed)

API Changes and Justification

Backwards Compatible Changes

  • This change introduces no API changes
  • This change adds new API calls

Backwards Incompatible Changes

  • This change modifies an existing API
  • This change removes an existing API

Review Status

cc @david-keitel @matthew-pitkin

Merge request reports