Fix: Fix rst based tests by MaximilianSoerenPollak · Pull Request #537 · eclipse-score/docs-as-code

MaximilianSoerenPollak · 2026-05-18T16:18:26Z

Expect-Not tests were not executed correctly and often were a no-op.
This change fixes that and makes them actually test things and relevant.

Changed behaviour of test checking too to enable more general usage.
Also changed all of the EXPECT-NOT to be generic instead of specific.

📌 Description

🚨 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-05-18T16:18:57Z

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.4.2) and connecting to it...
INFO: Invocation ID: 2b0deaf8-2bb3-480d-9553-de02ed1127c6
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
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.130s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
ERROR: Build failed. Not running target

github-actions · 2026-05-18T16:25:49Z

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

AlexanderLanin

AI-assisted review (Claude):

Critical: 0 | Important: 2 | Suggestions: 2

The core fix — switching filter_warnings_by_position from substring-matching to startswith-with-stripping — is correct and the ANSI-code scrubbing is a good catch. Two issues need attention before merging.

Key concerns:

Pre-filtering warnings to lines containing "score_metamodel" silently makes it impossible to assert on warnings from other Sphinx extensions (e.g., sphinx-needs itself). The one existing #EXPECT: unknown outgoing link test was a casualty of this — the warning comes from sphinx-needs, not score_metamodel, so it disappears before any matching occurs. The test was removed rather than the root cause being documented or addressed.
The std_wp__test__abcd EXPECT-NOT in test_id_contains_feature.rst was removed. If this EXPECT-NOT was previously a no-op (as the PR description says), and now fails after the fix, it means the "explicitly allowed" behavior is actually broken in production. Removing the test hides a real regression rather than fixing it.

AlexanderLanin · 2026-05-18T18:01:12Z

@@ -192,8 +207,13 @@ def test_rst_files(
    app.build()



AI-assisted review (Claude):

Important: Pre-filtering warnings to only "score_metamodel" lines is a semantic change that narrows the test scope silently. The removed #EXPECT: unknown outgoing link test (in test_metamodel_graph.rst) is a direct casualty: that warning originates from sphinx-needs, not score_metamodel, so it will never appear in this list. Any EXPECT annotation on a non-score_metamodel warning will now silently pass as a false positive.

If the intent is to restrict the test harness to score_metamodel warnings only, document that constraint clearly (e.g., a comment or docstring on the filter line), and explicitly state in the PR description that non-score_metamodel assertions are out of scope. The removed test should either be restored with a mechanism to include sphinx-needs warnings, or documented as intentionally dropped coverage.

It doesn't narrow the scope silently, it narrows the scope to exactly what we want to test.

I will double check the documentation to make sure this is mentioned clearly in there.

AlexanderLanin · 2026-05-18T18:01:14Z

@@ -62,17 +62,6 @@



AI-assisted review (Claude):

Important: This removes a positive test (#EXPECT) for link validation against a non-existent parent. The prior EXPECT-NOT tests were no-ops — this EXPECT test is different. Removing it drops the assertion that the rule produces a warning for unknown outgoing links entirely.

The likely root cause: the warning from sphinx-needs (unknown outgoing link) doesn't contain "score_metamodel", so it gets filtered out by the new pre-filter on line 208. But the fix should be to handle that at the filter level, not to silently drop test coverage.

This is a sphinx-needs warning / error. Not one of our own. I do not think we should start testing sphinx-needs functionality.
This did not work before either (as we filter on just errors coming from score_metamodel).

AlexanderLanin · 2026-05-18T18:01:28Z

@@ -13,13 +13,6 @@
   # *******************************************************************************


AI-assisted review (Claude):

Important: This test verified that "feature deeper in path is explicitly allowed" behavior. If the old EXPECT-NOT was a no-op (as claimed), the rule's behavior for this case was never actually verified. Two outcomes are possible:

The rule correctly does NOT warn → the test should be kept with a generic form like #EXPECT-NOT: Feature 'test' not in path

The rule incorrectly DOES warn → removing the test hides the regression instead of fixing the rule

Please verify by running the test locally with the fix applied. If the rule is correct, restore this test in the new generic style.

I could re-enable this test by renaming the need, or creating more folders.

As the current folder structure is not carried onto the error message (it's just '/' it doesn't go deeper than 1 folder).

What would be preferred here?

AlexanderLanin · 2026-05-18T18:01:30Z



-#EXPECT-NOT: feat_saf_fmea__test__good_2.sufficient (yes): does not follow pattern `^(yes|no)$`.
+#EXPECT-NOT: pattern


AI-assisted review (Claude):

Suggestion: "safety" and "pattern" are very broad substrings. For example, "safety" appears in many warning messages in this file — if the test fixture happens to generate an unrelated safety warning at the same line (e.g., from a different check), the EXPECT-NOT will silently pass regardless. A slightly more specific fragment — e.g., "does not follow pattern" or "safety level" — would remain generic enough to survive message rewording while being far less likely to match false positives.

This is a tradeoff the author can judge, but worth flagging for the safety-critical context of this codebase.

I'm unsure if making it more specific is needed / good.

In this line we would not expect ANY warning regarding pattern or safety. So I think keeping it broad makes more sense? Thoughts?

a-zw · 2026-05-19T05:58:22Z



-.. Parent requirement does not exist
-#EXPECT: unknown outgoing link


Why remove this test?

This is a sphinx-needs warning / error. Not one of our own. I do not think we should start testing sphinx-needs functionality.
This did not work before either (as we filter on just errors coming from score_metamodel).

MaximilianSoerenPollak · 2026-05-19T12:40:36Z

@FScholPer Just noticed a merge commit on this, was that by accident on this branch?

a-zw

LGTM

(maybe undo the main-merge for cleanliness)

Expect-Not tests were not executed correctly and often were a no-op. This change fixes that and makes them actually test things and relevant. Changed behaviour of test checking too to enable more general usage.

MaximilianSoerenPollak requested review from AlexanderLanin, dcalavrezo-qorix and nradakovic as code owners May 18, 2026 16:18

github-project-automation Bot added this to INF - Infrastructure Community May 18, 2026

github-project-automation Bot moved this to Backlog in INF - Infrastructure Community May 18, 2026

AlexanderLanin reviewed May 18, 2026

View reviewed changes

a-zw approved these changes May 19, 2026

View reviewed changes

Fix: Fix rst based tests

351c01f

Expect-Not tests were not executed correctly and often were a no-op. This change fixes that and makes them actually test things and relevant. Changed behaviour of test checking too to enable more general usage.

MaximilianSoerenPollak force-pushed the MSP_fix_rst_tests branch from 583e959 to 351c01f Compare May 19, 2026 15:22

AlexanderLanin approved these changes May 19, 2026

View reviewed changes

AlexanderLanin merged commit acc8ea9 into eclipse-score:main May 19, 2026
12 of 13 checks passed

github-project-automation Bot moved this from Backlog to Done in INF - Infrastructure Community May 19, 2026

		@@ -13,13 +13,6 @@
		# *******************************************************************************



		#EXPECT-NOT: feat_saf_fmea__test__good_2.sufficient (yes): does not follow pattern `^(yes\|no)$`.
		#EXPECT-NOT: pattern



		.. Parent requirement does not exist
		#EXPECT: unknown outgoing link

Conversation

MaximilianSoerenPollak commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📌 Description

🚨 Impact Analysis

✅ Checklist

Uh oh!

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

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

AlexanderLanin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaximilianSoerenPollak commented May 19, 2026

Uh oh!

a-zw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MaximilianSoerenPollak commented May 18, 2026 •

edited

Loading

github-actions Bot commented May 18, 2026 •

edited

Loading