test: stop asserting exception messages in EmailToken and TokenService tests - #8103
Open
maia-andre wants to merge 1 commit into
Open
test: stop asserting exception messages in EmailToken and TokenService tests#8103maia-andre wants to merge 1 commit into
maia-andre wants to merge 1 commit into
Conversation
…e tests Follow-up to LibreSign#8092. The review pointed out that asserting on translatable or easily mutable text couples the tests to strings that may be changed for non-technical reasons, forcing whoever edits the message to know the test must change too. Drop the three expectExceptionMessage() calls added in LibreSign#8092 and keep the exception class assertions. They are enough to distinguish the mutants: without ensureAvailable() the flow in TokenService reaches isGatewayComplete() and throws OCSForbiddenException instead of LibresignException, and the EmailToken scenarios still rely on the hasher outcome plus the exception class. Infection on both source files keeps Covered Code MSI at 100% (29 mutants, 0 escaped). Ref LibreSign#8053 Assisted-by: Claude Code:claude-fable-5 Signed-off-by: André Maia <andrefnkmm@gmail.com>
vitormattos
reviewed
Aug 28, 2026
| $instance->setCodeSentByUser('654321'); | ||
|
|
||
| $this->expectException(LibresignException::class); | ||
| $this->expectExceptionMessage('Invalid code.'); |
Member
There was a problem hiding this comment.
What does you think about add an error code to this exception?
With an error code we can confirm that is the expected error without matching the text.
Contributor
Author
There was a problem hiding this comment.
Agreed — I'll add a code so the tests assert expectExceptionCode() instead of the text. Two quick questions before I push:
- Should the code follow the "HTTP-like plus one digit" pattern you mentioned (e.g.
4010for the invalid verification code and5030for "Two-Factor Gateway not enabled"), and would you like them as constants, likeJSActions? - I'd apply it to both exceptions touched by this PR, unless you prefer only
Invalid code.for now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
Follow-up to #8092, addressing the review comments there: the tests added in that PR asserted on exception messages (
'Invalid code.'and'App Two-Factor Gateway is not enabled.'), which couples them to translatable/easily mutable text.This drops the three
expectExceptionMessage()calls and keeps the exception class assertions, which are enough to distinguish the mutants killed in #8092:TokenServiceTest: withoutensureAvailable()the flow reachesisGatewayComplete()and throwsOCSForbiddenException, which is not aLibresignException.EmailTokenTest: the scenarios still rely on the hasher outcome plus the exception class.Infection on both source files (
--testsuite=unit): 29 mutants, 0 escaped, Covered Code MSI 100%.The pre-existing assertion on
'Gateway sms not configured on Two-Factor Gateway.'was kept: it checks that the gateway name is interpolated into the message, which is behavior rather than wording. The broader question of how to assert exceptions across the suite (exception codes vs. typed subclasses) is being discussed in #8092.Backport notes: applies cleanly to stable35 (once #8102 is merged). Not applicable to stable32–34 —
TokenServiceTest.phpdoes not exist there.Related Issue
Ref #8053
Pull Request Type
Pull request checklist