test(manifest): add comprehensive unit tests for test configuration parsing#1266
test(manifest): add comprehensive unit tests for test configuration parsing#1266ZainabTravadi wants to merge 2 commits intofortran-lang:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens the manifest module’s unit coverage around parsing [[test]] entries (new_test), aiming to validate parsed values, defaults, and invalid-name handling.
Changes:
- Adds new unit tests covering full-field parsing, defaulting behavior, and invalid test names.
- Refactors a few existing test lines for formatting/readability.
- Updates the
test_valid_manifestTOML fixture forsource-dir(noted as a behavioral change in the test input).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end if | ||
|
|
||
| if (index(parse_error%message, 'name must be composed only of') <= 0) then | ||
| call test_failed(error, 'Expected invalid-name phrase in error message') |
There was a problem hiding this comment.
When the error-message substring check fails, the test currently reports a generic message. Including parse_error%message (or at least a larger expected substring) in the failure output would make diagnosing future regressions much easier.
| call test_failed(error, 'Expected invalid-name phrase in error message') | |
| call test_failed(error, 'Expected invalid-name phrase in error message, got: '//parse_error%message) |
| & '[''library'']', & | ||
| & 'source-dir = """', & | ||
| & 'lib""" # comment', & | ||
| & 'source-dir = "lib" # comment', & |
There was a problem hiding this comment.
The test_valid_manifest fixture used to exercise TOML multiline basic string parsing for source-dir (with closing delimiter followed by a comment). Collapsing it to a single-line string drops that coverage and isn’t mentioned in the PR description; consider keeping the multiline-string case (or adding a dedicated test) if it’s still supported/important.
| & new_unittest("test-typeerror", test_test_typeerror, should_fail=.true.), & | ||
| & new_unittest("test-noname", test_test_noname, should_fail=.true.), & | ||
| & new_unittest("test-wrongkey", test_test_wrongkey, should_fail=.true.), & | ||
| & new_unittest("test-test-fullparse", test_test_fullparse), & |
There was a problem hiding this comment.
The unittest name "test-test-fullparse" has a duplicated test- prefix and is inconsistent with the other test-case names in this section (e.g., test-simple, test-empty, test-defaults). Consider renaming it to something like test-fullparse for consistency.
| & new_unittest("test-test-fullparse", test_test_fullparse), & | |
| & new_unittest("test-fullparse", test_test_fullparse), & |
| call test_failed(error, 'Expected dependencies to be parsed and allocated') | ||
| return | ||
| end if | ||
|
|
There was a problem hiding this comment.
test_test_fullparse sets up a dependency (dummydep with a path) but only asserts that test%dependency is allocated. To match the stated goal of verifying parsed field values, this test should also assert the dependency count and key fields (e.g., that dummydep exists and its path was parsed correctly).
| if (size(test%dependency) /= 1) then | |
| call test_failed(error, 'Expected exactly one dependency to be parsed') | |
| return | |
| end if | |
| call check_string(error, test%dependency(1)%name%s, 'dummydep', & | |
| 'Test dependency name') | |
| if (allocated(error)) return | |
| call check_string(error, test%dependency(1)%path%s, './dummydep', & | |
| 'Test dependency path') | |
| if (allocated(error)) return |
…erage - Add assertions for dependency count and parsed values - Ensure correct handling of character fields (no string_t usage) - Improve validation in fullparse test
Closes #1267
Summary
Add comprehensive unit tests for
new_testparsing in the manifest module.The current test suite validates basic success/failure cases but does not
fully verify parsed field values or default behavior. This PR strengthens
coverage by asserting actual parsed outputs and edge cases.
Changes
Added three new tests:
test_test_fullparse
namesource-dirmainlinkdependenciestest_test_defaults
source-dir = "test"main = "main.f90"test_test_invalid_name
Why this matters
These tests establish a reliable baseline for test configuration parsing,
which is directly relevant to upcoming improvements in manifest handling.
They also improve confidence in:
Scope