Skip to content

Conversation

@koshchii
Copy link
Member

@koshchii koshchii commented Dec 19, 2025

Description of work

This PR follows-up on a conversation we had during the last Developer Meeting, where it was agreed to replace the manual testing of the TOFTOF DGS Reduction GUI by an appropriate system test, which reproduces the manual testing workflow.

Closes #40263.

To test:

Run the new TOFTOFReductionTest system test (./systemtest -R TOFTOFReductionTest) from your build folder and make sure that the test passes checks.


Reviewer

Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.

As per the review guidelines:

  • Is the code of an acceptable quality? (Code standards/GUI standards)
  • Has a thorough functional test been performed? Do the changes handle unexpected input/situations?
  • Are appropriately scoped unit and/or system tests provided?
  • Do the release notes conform to the guidelines and describe the changes appropriately?
  • Has the relevant (user and developer) documentation been added/updated?
  • If the PR author isn’t in the mantid-developers or mantid-contributors teams, add a review comment rerun ci to authorize/rerun the CI

Gatekeeper

As per the gatekeeping guidelines:

  • Has a thorough first line review been conducted, including functional testing?
  • At a high-level, is the code quality sufficient?
  • Are the base, milestone and labels correct?

@koshchii koshchii added this to the Release 6.16 milestone Dec 19, 2025
@koshchii koshchii added the Maintenance Unassigned issues to be addressed in the next maintenance period. label Dec 19, 2025
@koshchii koshchii force-pushed the dgs_reduction_systemtest branch from 68ae827 to 1885778 Compare December 19, 2025 09:08
@koshchii
Copy link
Member Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Tests

    • Added new system test for TOFTOF reduction workflow with validation datasets and reference outputs.
  • Documentation

    • Removed DGS reduction testing documentation page from testing guides.

✏️ Tip: You can customize this high-level summary in your review settings.

Walkthrough

The changes implement a new system test for TOFTOF reduction workflows to replace manual GUI testing procedures. Multiple MD5 checksum files are added for test data validation across both input and reference output datasets. A new TOFTOFReductionTest class is introduced to the system test framework with setup, execution, and validation methods. The test framework configuration is updated to register the new test module. Associated documentation for the manual DGSReduction test process is removed from the developer documentation.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main objective of the PR: replacing manual TOFTOF DGS reduction testing with an automated system test.
Description check ✅ Passed The description clearly relates to the changeset by explaining the motivation, referencing the linked issue #40263, and providing testing instructions for the new system test.
Linked Issues check ✅ Passed The PR implements the core requirements from #40263: creates a system test for TOFTOF DGS reduction workflow, uses the full MLZ sample data set, and includes result verification with reference checksums.
Out of Scope Changes check ✅ Passed All changes are in scope: new system test files, test data checksums, CMakeLists.txt addition for test registration, and removal of obsolete manual testing documentation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2b6d502 and 1885778.

📒 Files selected for processing (19)
  • Testing/Data/SystemTest/TOFTOF12.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF13.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF14.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF15.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF16.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF17.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF27.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF28.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF29.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF30.nxs.md5 (1 hunks)
  • Testing/Data/SystemTest/TOFTOF31.nxs.md5 (1 hunks)
  • Testing/SystemTests/tests/framework/CMakeLists.txt (1 hunks)
  • Testing/SystemTests/tests/framework/TOFTOFReductionTest.py (1 hunks)
  • Testing/SystemTests/tests/framework/reference/TOFTOF_EC_1517_processed.nxs.md5 (1 hunks)
  • Testing/SystemTests/tests/framework/reference/TOFTOF_Sample_2729_processed.nxs.md5 (1 hunks)
  • Testing/SystemTests/tests/framework/reference/TOFTOF_Sample_3031_processed.nxs.md5 (1 hunks)
  • Testing/SystemTests/tests/framework/reference/TOFTOF_Van_1214_processed.nxs.md5 (1 hunks)
  • dev-docs/source/Testing/Direct/DGSReductionTests.rst (0 hunks)
  • dev-docs/source/Testing/index.rst (0 hunks)
💤 Files with no reviewable changes (2)
  • dev-docs/source/Testing/index.rst
  • dev-docs/source/Testing/Direct/DGSReductionTests.rst
🔇 Additional comments (23)
Testing/Data/SystemTest/TOFTOF13.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF31.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF29.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF12.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF17.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF16.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/Data/SystemTest/TOFTOF27.nxs.md5 (1)

1-1: Verify input data checksum is correctly integrated into test.

This checksum file is a test data artifact. Without access to the test implementation (TOFTOFReductionTest.py), I cannot verify that this checksum is properly referenced and used for data validation in the system test.

Testing/SystemTests/tests/framework/reference/TOFTOF_Sample_3031_processed.nxs.md5 (1)

1-1: Verify reference checksum is correctly used for result validation in test.

This is a reference checksum for expected output validation. The PR objectives mention verifying numerical values of resulting reduced workspaces. Without seeing the test implementation (TOFTOFReductionTest.py), I cannot verify:

  • This checksum is properly loaded and used in assertions
  • Whether MD5 validation alone is sufficient, or if numerical value checks are also performed
Testing/SystemTests/tests/framework/CMakeLists.txt (1)

211-211: LGTM!

The new test module is correctly added to the SYSTEST_NAMES set in proper alphabetical order, consistent with the existing entries.

Testing/SystemTests/tests/framework/reference/TOFTOF_EC_1517_processed.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum format for the Empty Can reference data. The naming convention is consistent with other reference files in this PR.

Testing/Data/SystemTest/TOFTOF15.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum for input test data file TOFTOF15.nxs.

Testing/Data/SystemTest/TOFTOF30.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum for input test data file TOFTOF30.nxs.

Testing/Data/SystemTest/TOFTOF14.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum for input test data file TOFTOF14.nxs.

Testing/SystemTests/tests/framework/reference/TOFTOF_Van_1214_processed.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum format for the Vanadium reference data. The naming convention is consistent with other reference files in this PR.

Testing/Data/SystemTest/TOFTOF28.nxs.md5 (1)

1-1: LGTM!

Valid MD5 checksum for input test data file TOFTOF28.nxs.

Testing/SystemTests/tests/framework/reference/TOFTOF_Sample_2729_processed.nxs.md5 (1)

1-1: LGTM! Valid MD5 reference checksum.

The checksum format is correct and will be used to validate the processed output workspace from the TOFTOF reduction test.

Testing/SystemTests/tests/framework/TOFTOFReductionTest.py (7)

1-18: LGTM! Well-structured test class with clear documentation.

The imports are appropriate for a TOFTOF reduction system test, and the class correctly inherits from MantidSystemTest. The docstring clearly explains the test's purpose of replacing manual DGS Reduction steps.


28-41: LGTM! Required files correctly listed.

The file list properly covers all the data files needed for the reduction workflow: vanadium runs (12-14), empty container runs (15-17), and sample data runs (27-31). This matches the run ranges specified in the reduction script configuration.


43-45: LGTM! Clean test execution flow.

The method correctly separates script generation from execution, which makes the test easy to understand and maintain.


47-61: LGTM! Comprehensive validation configuration.

The validation setup properly checks all four expected output workspaces (vanadium, empty container, and two sample temperatures) against their reference files. The 0.1% relative tolerance, NaN equality, and axes checking disabled are appropriate choices for comparing reduced S(Q,ω) data.


90-119: LGTM! Reduction parameters properly configured.

The binning, normalization, and TOF correction parameters are well-configured for TOFTOF reduction. All save options are appropriately disabled since the test validates in-memory workspaces rather than saved files. The inline comments for the numeric options (normalize and correctTof) make the configuration clear.


20-26: Verify the setUp() call pattern in init.

The __init__ method explicitly calls self.setUp(), which is unusual since many test frameworks (including unittest) call setUp() automatically before each test method. This could potentially cause setUp() to be executed twice.

Please verify whether this is the correct pattern for MantidSystemTest. If the framework already calls setUp() automatically, this explicit call may be redundant or could cause issues.

#!/bin/bash
# Description: Search for other system tests that call setUp() in __init__ to verify this is a standard pattern

# Search for pattern of calling setUp in __init__ within system tests
rg -n -A3 'def __init__.*:' Testing/SystemTests/tests/framework/*.py | rg -A3 'setUp\(\)'

63-88: Configuration parameters are correct.

The reduction script configuration properly sets up vanadium, empty container, and sample data runs matching the required files. The detector masking list and temperature parameters align with the manual test workflow. The empty dataDir is the framework's default initialization in TOFTOFScriptElement and is correctly used here.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Maintenance Unassigned issues to be addressed in the next maintenance period.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TOFTOF DGSReduction GUI Manual tests to system test

1 participant