Feat: Refactor RST tests by MaximilianSoerenPollak · Pull Request #614 · eclipse-score/docs-as-code

MaximilianSoerenPollak · 2026-06-24T10:47:30Z

📌 Description

Completely refactor RST tests on how they work.
Added test_metadata to ensure linking is possible in them as well

🚨 Impact Analysis

This change does not violate any tool requirements and is covered by existing tool requirements
This change does not violate any design decisions
Otherwise I have created a ticket for new tool qualification

✅ Checklist

Added/updated documentation for new or changed features
Added/updated tests to cover the changes
Followed project coding standards and guidelines

github-actions · 2026-06-24T10:48:00Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //src:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 0a702942-cf0a-4610-af3b-9833fa7e43bc
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: src
WARNING: Target pattern parsing failed.
ERROR: Skipping '//src:license-check': no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
ERROR: no such target '//src:license-check': target 'license-check' not declared in package 'src' defined by /home/runner/work/docs-as-code/docs-as-code/src/BUILD
INFO: Elapsed time: 5.400s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

AlexanderLanin · 2026-06-24T10:54:10Z

+    # Build the documentation
+    app = sphinx_app_setup(RST_DIR / rst_file)
+    monkeypatch.chdir(app.srcdir)  # Sphinx resolves paths relative to the source dir
    app.build()


AI-assisted review (Claude):

Critical: check filter is set after app.build() — checks run with the wrong filter

The score_metamodel_checks config is set from the test_metadata need at line 318–319, but app.build() is called at line 308. The metamodel check runner (_run_checks) fires on the build-finished event, which is emitted inside app.build(). So the check filter is always empty when checks actually run — every file runs all checks, not just the one declared in :check:.

The old code did this correctly: app.config.score_metamodel_checks = rst_data.enabled_checks was set before app.build().

The fix requires reading the :check: value before building. One approach: parse it from the RST source (the old #CHECK: comment pattern) or use a two-pass approach (build once to discover metadata, then build again with the filter). Alternatively, keep the :check: value in conf.py and pass it via confoverrides to SphinxTestApp.

AlexanderLanin · 2026-06-24T10:54:31Z

+.. test_metadata::
+   :id: test_metadata__id_contains_feature
+   :partially_verifies_list: tool_req__docs_common_attr_id_scheme
+   :test_type: requirements_test


AI-assisted review (Claude):

Important: "requirements_test" is not a valid TestType — runtime KeyError guaranteed

The TestType literal in attribute_plugin.py accepts: "fault-injection", "interface-test", "requirements-based", "resource-usage". The value here is requirements_test (underscore, no hyphen, wrong suffix), which matches none of them.

_build_test_properties receives this as test_type and passes it directly to pytest.mark.test_properties. Depending on downstream validation, this either silently produces wrong XML output or fails at runtime when the report generator validates the value.

Fix: change to requirements-based.

AlexanderLanin

AI-assisted review (Claude):

Critical: 2 | Important: 1 | Suggestions: 2

The refactor has solid motivation — moving test metadata from ad-hoc #CHECK: / #EXPECT comments into first-class test_metadata needs is a real improvement. The code structure is cleaner and the approach of attaching traceability links to RST-based tests is the right direction.

However there are two critical regressions that break the core mechanism:

app.build() is called before score_metamodel_checks is set (line 308 vs 318–319). The check filter fires inside build-finished, so every test file runs all checks, not just the one declared in :check:. The old code set the config before building. This needs to be read from RST before the app is constructed — either by retaining a lightweight pre-parse of the :check: value, or by threading it through confoverrides in the SphinxTestApp constructor.
metadata_need.get("CHECK") uses the wrong case — the field is "check" (lowercase). This always returns None, compounding issue #1 (the filter is always empty).

Together these mean the check-scoping that was present in the old code is completely lost in this PR.

One additional important issue: the requirements_test value in test_id_contains_feature.rst is not in the TestType literal — the valid value is requirements-based.

The -s -vv debug args committed to all five BUILD targets should also be removed before merge.

AlexanderLanin

AI-assisted review (Claude):

Re-review after new commit (faefabd). The only change since the last review is an addition to docs/internals/extensions/rst_filebased_testing.md.

None of the previously reported issues have been addressed. All five original findings remain open:

❌ Critical — app.build() called before score_metamodel_checks is set (line 308 vs 318)
❌ Critical — metadata_need.get("CHECK") uses wrong case; field is "check" (line 318)
❌ Important — test_id_contains_feature.rst uses requirements_test, not the valid requirements-based
❌ Important — -s -vv debug flags left in all five BUILD targets
❌ Suggestion — apply_test_metadata_props duplicates apply_test_metadata from attribute_plugin.py
❌ Suggestion — conf.py guard "test_metadata" not in all_needs_types checks a string against a list of dicts; always true

The two Criticals are blockers — with the current code the check filter is never applied during the build, so the per-file check scoping introduced by the test_metadata need has no actual effect.

a-zw · 2026-06-24T12:24:22Z

    results.clear()
    # As kwargs ordering is deterministic this will always put the first total into results[0]
    _get_key_values(results, [str(value) for value in kwargs.values()])
+


Why are there random changes like this one?

Formatting.
That was a change the pre-commit did when I ran pre-commit run -a.

github-actions · 2026-06-24T16:33:46Z

The created documentation from the pull request is available at: docu-html

Copilot

Pull request overview

This PR refactors the Sphinx RST “file-based” tests to use real Sphinx-Needs directives for expectations and introduces test_metadata so test cases can be linked/annotated (e.g., in junitxml) with requirement verification metadata.

Changes:

Refactors the file-based test runner to validate :expect: / :expect_not: directly on needs and to attach runtime metadata to junitxml output.
Updates most RST test inputs to include a test_metadata need and to move expectations onto the tested needs.
Extends the test-only Sphinx conf.py to support the new test fields/options and updates documentation accordingly.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
src/extensions/score_metrics/sphinx_filters.py	Minor formatting tweak (blank line).
src/extensions/score_metamodel/tests/test_rules_file_based.py	Refactors RST test execution and adds junitxml metadata application.
src/extensions/score_metamodel/tests/rst/options/wp_comp.rst	Migrates expectations to `:expect:` / `:expect_not:` and adds `test_metadata`.
src/extensions/score_metamodel/tests/rst/options/test_options_options.rst	Migrates expectations to `:expect:` / `:expect_not:` and adds `test_metadata`.
src/extensions/score_metamodel/tests/rst/options/test_options_extra_option.rst	Migrates expectations to `:expect:` / `:expect_not:` and adds `test_metadata`.
src/extensions/score_metamodel/tests/rst/options/test_need_extends.rst	Updates needextend scenarios/expectations.
src/extensions/score_metamodel/tests/rst/options/gd_req_comp.rst	Adds `test_metadata` and migrates expectations (partially disables a case).
src/extensions/score_metamodel/tests/rst/id_contains_feature/test_id_contains_feature.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/graph/test_workproduct_aspice_40.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/graph/test_metamodel_graph.rst	Adds `test_metadata` and migrates expectations; removes a large block of cases.
src/extensions/score_metamodel/tests/rst/graph/test_invalid_graph.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/conf.py	Adds test-only needs fields/need type wiring for the new test scheme.
src/extensions/score_metamodel/tests/rst/attributes/test_validity.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/attributes/test_prohibited_words.rst	Adds `test_metadata` and migrates some expectations to need options.
src/extensions/score_metamodel/tests/rst/attributes/test_attributes_format_id_length.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/attributes/test_attributes_format_id_format.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/attributes/test_attributes_external_prefix.rst	Adds `test_metadata` and migrates expectations to need options.
src/extensions/score_metamodel/tests/rst/architecture/architecture_tests.rst	Adds `test_metadata` at the top of the file.
src/extensions/score_metamodel/metamodel.yaml	Removes a graph check block and adjusts spacing.
src/extensions/score_metamodel/BUILD	Adds `tests/__init__.py` to Bazel test data for multiple targets.
score_pytest/attribute_plugin.py	Refactors metadata property building and adds runtime metadata application helper.
docs/internals/requirements/requirements.rst	Adds/updates tool requirements related to workproduct/standard types.
docs/internals/extensions/rst_filebased_testing.md	Updates documentation to match the new RST test approach.

+def _get_default_metadata_need() -> NeedItem:
+    return test_need(
+        fully_verifies=[],
+        partially_verifies=[],
+        test_type="",
+        line_nr="",
+        file="",
+        derivation_technique="",
+        descdription="",
+    )


+    line_nr = int(str(rst_data.metadata["line_nr"]))
+    file_name = clean_filepath(request) + str(rst_data.metadata["file"])
+    apply_test_metadata(


+    add_options_regex = {
+        "check": "^.*$",
+        "expect_not": "^.*$",
+        "expect": "^.*$",
+        "fully_verifies_list": "^.*$",
+        "partially_verifies_list": "^.*$",
+    }


   :id: stkh_req__random_id1
   :valid_from: v1.0.1
   :valid_until: v0.5
+   :expect: stkh_req__random_id1: inconsistent validity: valid_from (v1.0.1) >= valid_until (v0.5



+
 .. stkh_req:: Test Req Extends 1


 .. feat_req:: This is a test
   :id: feat_req__test__title_good
+   :expect_not: title contains



 .. stkh_req:: This is a test
-   :id: stkh_req__test_title_good
+   :id: stkh_req__test__title_good
+   :expect_not: title contains



 It uses the SphinxTestApp to build the documentation and checks for the
 **expected/not expected** warnings.

+**It has it's OWN conf.py, so if changes need to be made ensure they are made in there**


+This example verifies that the warning message
+*std_wp__test__abcd: is missing required attribute: \`status\`* is shown during
+the Sphinx build. Only the *check_options* check is enabled.


 Graph checks (defined in `metamodel.yaml` under `graph_checks:`) are all executed
-by a single Python function `check_metamodel_graph`. Use that function name in `#CHECK:`:
+by a single Python function `check_metamodel_graph`. Use that function name in the
+`test_metadata` need's `:check:` option:
+


AlexanderLanin · 2026-06-24T18:40:04Z

+.. test_metadata::
+   :id: test_metadata__id_contains_feature
+   :partially_verifies_list: tool_req__docs_common_attr_id_scheme
+   :test_type: requirements_test


AI-assisted review (Claude):

Important: "requirements_test" invalid test_type — still not fixed

test_id_contains_feature.rst line 19 still has :test_type: requirements_test. The valid TestType values are "fault-injection", "interface-test", "requirements-based", "resource-usage". This value will be written to the JUnit XML as-is and silently produces a non-conforming test result.

Fix: change to requirements-based.

AlexanderLanin · 2026-06-24T18:40:15Z

+        },
+    }
+    add_options_regex = {
+        "check": "^.*$",


AI-assisted review (Claude):

Suggestion: :check: field is dead code — registered but never used

The "check" field is defined in add_needs_fields and add_options_regex in conf.py, and parse_test_metadata stores it (need.get("check")). But after removing :check: from all RST files (commit 0b56605), nothing reads metadata["check"] — neither test_rules_file_based.py nor apply_test_metadata.

Remove it from add_needs_fields, add_options_regex, parse_test_metadata, and the test_metadata optional_options to avoid confusion. If per-file check scoping is re-introduced later, it should be re-added intentionally.

AlexanderLanin · 2026-06-24T18:40:24Z

 Graph checks (defined in `metamodel.yaml` under `graph_checks:`) are all executed
-by a single Python function `check_metamodel_graph`. Use that function name in `#CHECK:`:
+by a single Python function `check_metamodel_graph`. Use that function name in the
+`test_metadata` need's `:check:` option:


AI-assisted review (Claude):

Suggestion: documentation still describes :check: as a working feature

Lines 44–46 describe <check functions> as a field the user can set, and lines 106–114 show a .. test_metadata:: example with :check: check_metamodel_graph. But :check: was removed from all RST test files in commit 0b56605, and the test runner no longer reads it. A contributor following this doc would add :check: to their RST file and wonder why check scoping has no effect.

Either remove the :check: references from the doc (and the example), or re-implement the feature properly before documenting it.

AlexanderLanin

AI-assisted review (Claude):

Re-review after commits 0b56605–9ca8937.

Critical: 0 | Important: 1 | Suggestions: 2

Good progress — both critical build-ordering issues from the first review are resolved by removing the per-file :check: scoping entirely (commit 0b56605). The debug flags, duplicate function, and conf.py guard are also fixed. The PR is substantially better.

Still open from previous reviews:

❌ Important — :test_type: requirements_test in test_id_contains_feature.rst is still there. requirements_test is not a valid TestType literal. Change to requirements-based.

New findings:

⚠️ Suggestion — The "check" field is now dead code: registered in conf.py, populated in parse_test_metadata, but never read by anything since :check: was removed from all RST files. Clean it up.
⚠️ Suggestion — The documentation still describes :check: as a feature (lines 44–46 and 106–114 of rst_filebased_testing.md), with example code blocks showing how to use it. Since the feature no longer works, this will mislead contributors. Either remove the references or re-implement the feature.

Other issues (typo in descdription, test_need_extends.rst missing test_metadata, safety: YES invalid values) are already flagged by other reviewers — I won't duplicate them.

MaximilianSoerenPollak added 2 commits June 23, 2026 10:41

WIP: Refactor rst tests

d1346f8

Feat: Refactor RST Tests

96ec97c

MaximilianSoerenPollak requested review from AlexanderLanin, dcalavrezo-qorix and nradakovic as code owners June 24, 2026 10:47

github-project-automation Bot added this to INF - Infrastructure Community Jun 24, 2026

AlexanderLanin reviewed Jun 24, 2026

View reviewed changes

Comment thread src/extensions/score_metamodel/tests/test_rules_file_based.py Outdated

AlexanderLanin reviewed Jun 24, 2026

View reviewed changes

Comment thread src/extensions/score_metamodel/BUILD Outdated

AlexanderLanin reviewed Jun 24, 2026

View reviewed changes

Comment thread src/extensions/score_metamodel/tests/test_rules_file_based.py Outdated

AlexanderLanin reviewed Jun 24, 2026

View reviewed changes

Comment thread src/extensions/score_metamodel/tests/rst/conf.py Outdated

AlexanderLanin requested changes Jun 24, 2026

View reviewed changes

MaximilianSoerenPollak added 2 commits June 24, 2026 12:56

Feat: Add documentation for rst based testing

b3104a8

Chore: Formating & Linting

faefabd

AlexanderLanin requested changes Jun 24, 2026

View reviewed changes

a-zw reviewed Jun 24, 2026

View reviewed changes

MaximilianSoerenPollak added 6 commits June 24, 2026 15:34

Fix: Remove 'check' options now runs all checks

0b56605

Chore: Fix documentation rendering

c143889

Fix: Adress PR comments and issues

9fe8ab2

Chore: Linting & Formatting

ba8d975

Fix: Fix build errors & simplify logic

e29c17a

Chore: Linting & Formatting

9ca8937

MaximilianSoerenPollak requested a review from AlexanderLanin June 24, 2026 16:30

MaximilianSoerenPollak requested a review from Copilot June 24, 2026 16:36

Copilot started reviewing on behalf of MaximilianSoerenPollak June 24, 2026 16:36 View session

Copilot AI reviewed Jun 24, 2026

View reviewed changes

AlexanderLanin reviewed Jun 24, 2026

View reviewed changes

AlexanderLanin requested changes Jun 24, 2026

View reviewed changes

Uh oh!

Conversation

MaximilianSoerenPollak commented Jun 24, 2026

📌 Description

🚨 Impact Analysis

✅ Checklist

Uh oh!

github-actions Bot commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

AlexanderLanin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexanderLanin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexanderLanin left a comment

Choose a reason for hiding this comment

Uh oh!

AlexanderLanin left a comment

Choose a reason for hiding this comment

Uh oh!

a-zw Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

MaximilianSoerenPollak Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

AlexanderLanin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

AlexanderLanin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

AlexanderLanin Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

AlexanderLanin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Jun 24, 2026 •

edited

Loading