Problem/Motivation
WebformSubmissionResultTest::normalizedElementIdTest() is named with a Test suffix instead of a test prefix, and carries no @test / #[Test] annotation. PHPUnit only collects methods whose name starts with test (or are annotated), so this method is never run.
It is meant to be the only coverage for element-ID normalization across addValidationError(), getOrCreateValidationError() and hasValidationError() (it carries the matching @covers tags). In practice it contributes nothing.
Reviving it also exposes a behavior bug: with the method renamed so it runs, it fails at the existence check that uses the original (un-normalized) element ID, because getOrCreateValidationError() does not store the error it creates. So the test has been hiding a real defect, not just sitting idle.
Steps to reproduce
Rename the method to a test-prefixed name and run the unit test:
ddev phpunit web/modules/custom/graphql_webform/tests/src/Unit/WebformSubmissionResultTest.phpIt fails with:
The element ID was normalized when checking for a validation error. Failed asserting that false is true. tests/src/Unit/WebformSubmissionResultTest.php (hasValidationError assertion)
Proposed resolution
Rename normalizedElementIdTest() to testNormalizedElementId() so PHPUnit runs it, then make the test pass by resolving the underlying behavior.
The failure is that getOrCreateValidationError() returns a fresh error object without storing it in $this->validationErrors (only addValidationError() stores), so hasValidationError() reports false after a get-or-create. Pick one:
- Option A (keep model behavior): register the error through
addValidationError()before asserting existence, and drop the assumption that get-or-create persists. - Option B (match the "or create" naming): have
getOrCreateValidationError()store the created error. Note this changesisValid(), which returnsfalsewhenever$this->validationErrorsis non-empty; a stored empty error would then mark the submission invalid, soisValid()would need to ignore errors that have no messages.
Remaining tasks
- Rename the method to
testNormalizedElementId(). - Choose Option A or B and implement it in
src/Model/WebformSubmissionResult.phpand/or the test. - Confirm phpunit, phpcs and phpstan pass.
Issue fork graphql_webform-3600702
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #4
kieran.cottI've gone with Option B, because I think it's better than methods are named after what they do. This is a little bit more involved than Option A but I think it'll be less confusing to maintain going forward.
Comment #6
kieran.cottPipeline pass on MR 89.
Comment #7
pfrenssenThanks very much, test passes again!
Comment #9
pfrenssen