Skip to content

Integrate tests: maintainability of the validation scripts (monolithic catch_properties.sh, unclear tool layout) #8091

Description

@mohanchen

Describe the Code Quality Issue

Problem

While working on the integrate-test tooling in tests/integrate/, we hit
several maintenance pain points:

  1. A 1000+-line monolithic collector. tests/integrate/tools/catch_properties.sh
    collects every kind of result line (energies, forces, stresses, DOS,
    matrices, cubes, ML, DeePKS, TDDFT, ...) into result.out in a single
    file. Finding where a given key is produced is slow, and adding a new
    kind of check means editing an ever-growing file whose output-order
    constraints are only implicit.

  2. Duplicated helpers. catch_deepks_properties.sh re-defines helpers
    (sum_file, get_input_key_value, compare-script path) that also exist
    in catch_properties.sh, so the two copies can drift apart silently.

  3. Hard-coded relative paths. Scripts reference helpers through
    ../../integrate/tools/... relative to the test-case working
    directory, which breaks whenever the collector is invoked with a
    different CWD and makes the scripts fragile to relocation.

  4. Dead code. E.g. the compare-wfc subcommand of cube_tool.py has
    no caller, and the collector contains unreachable branches.

  5. Awkward executable selection for local runs. Pointing the tests at
    a local ABACUS build requires either exporting ABACUS_EXE in every
    new shell or editing tracked files (Autotest.sh, general_info),
    which risks committing a machine-specific absolute path.

  6. No documentation of the validation workflow. How result.ref /
    result.out comparison works (thresholds, per-case threshold files,
    the information-only totaltimeref line) is not written down anywhere;
    the directory name tools also does not convey that these scripts are
    the validation toolkit.

Proposal

  • Split the collector into one module per result family
    (props_basic.sh, props_mat.sh, props_cube.sh, props_ml.sh,
    props_tddft.sh, props_deepks.sh) plus a shared props_common.sh,
    each exposing run_<category>_props() hooks; catch_properties.sh
    becomes a thin orchestrator that calls the hooks in the original order.

  • Rename tests/integrate/tools to tests/integrate/validation_tools
    and resolve helper paths from the script's own location.

  • Support an untracked, gitignored integrate/general_info.local as a
    local executable override (priority: -a flag > ABACUS_EXE >
    local file > PATH), on top of the existing ABACUS_EXE support.

  • Document the validation workflow (three entry points: whole category,
    single case, collector-only) in tests/README.

  • Remove the dead code.

Constraints

  • The collected output must not change: result.out content and exit
    codes stay byte-for-byte identical for all existing cases (verified by
    running the old and new collectors side by side; only totaltimeref,
    the wall-time line, differs by design).

  • No INPUT parameter behavior is involved, and CI (which only invokes
    Autotest.sh) keeps working unchanged.

Additional Context

No response

Task list for Issue attackers (only for developers)

  • Identify the specific code file or section with the code quality issue.
  • Investigate the issue and determine the root cause.
  • Research best practices and potential solutions for the identified issue.
  • Refactor the code to improve code quality, following the suggested solution.
  • Ensure the refactored code adheres to the project's coding standards.
  • Test the refactored code to ensure it functions as expected.
  • Update any relevant documentation, if necessary.
  • Submit a pull request with the refactored code and a description of the changes made.

Activity

  1. added
    RefactorRefactor ABACUS codes
    Tests/ExamplesIssues/PR related to unit tests and integrate tests
    on Oct 7, 2026
  2. Growl1234 commented on Oct 7, 2026

    @Growl1234

    IMHO splitting one file to many just because it has too many lines is not a good idea; it instead increases the complexity of code/script structure and will more likely intimidate newcomers. The Toolchain configuration scripts (scripts/lib) in ABACUS is already a typical counter-example.

    I would rather hope there're just at most 2-3 scripts to well process the integration tests, as the test handling is just some machinary works. So instead, we should do proper dedup, clean-up and prettifying to make the script as clear as possible. Also, it might be a good idea to use Python scripts to handle the tests.

  3. mohanchen commented on Oct 8, 2026

    @mohanchen
    CollaboratorAuthor

    IMHO splitting one file to many just because it has too many lines is not a good idea; it instead increases the complexity of code/script structure and will more likely intimidate newcomers. The Toolchain configuration scripts (scripts/lib) in ABACUS is already a typical counter-example.

    I would rather hope there're just at most 2-3 scripts to well process the integration tests, as the test handling is just some machinary works. So instead, we should do proper dedup, clean-up and prettifying to make the script as clear as possible. Also, it might be a good idea to use Python scripts to handle the tests.

    Thanks for your advice. Sorry, I will not accept your advice at this point.

  4. added a commit that references this issue on Oct 10, 2026
    97697c2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    RefactorRefactor ABACUS codesTests/ExamplesIssues/PR related to unit tests and integrate tests

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions