LibreSign / LibreSign/libresign
Stop asserting exception messages in PHP tests: define a convention (error codes or typed exceptions)
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 818
- Forks
- 146
- Avg merge
- 11h 31m
- Merged PRs (30d)
- 326
Description
Follow-up of the discussion in #8092 (https://github.com/LibreSign/libresign/pull/8092#discussion_r3883064993).
Many PHP tests assert exceptions by matching their message. When the message is translatable or just easy to change, editing the text in lib/ silently requires editing the test too, and whoever changes the text (not necessarily a developer) has to know that. The suggestion in #8092 was to assert the error code or the exception class instead of the text.
Current state
Numbers from main today:
expectExceptionMessage(): 174 calls in 52 test files (plus 13expectExceptionMessageMatches()).expectExceptionCode(): 12 calls in 9 files.throw new LibresignException(...): 287 places. 161 of them use a translated message ($l10n->t(...)), 126 use a literal string.- Only 51 of the 287 carry a code, and the values mix conventions:
1(22),422(11),404(7),500(4),400(3),Http::STATUS_UNPROCESSABLE_ENTITY(3) andHttp::STATUS_FORBIDDEN(1).LibresignExceptionitself defines no code constants, and the code is exposed to clients throughErrorPayloadBuilder(code) andSignFileService(error_code), so any convention also touches the API contract. - 8 test files assert a translated message while building the real
IL10Nfrom the factory, so they depend on the test locale. - There are already typed subclasses in
lib/Exception/(InvalidPasswordException,InvalidSignatureException,EmptyCertificateException, ...), but mostthrowsites still use the baseLibresignException.
What should be decided first
Two directions came up in #8092. They are not exclusive, but a convention is needed before touching many files:
- Error codes: define constants for the codes (a dedicated class, like
JSActions, following the "HTTP-like plus one digit" idea to avoid confusion with HTTP status codes), use them at thethrowsites, and assert them withexpectExceptionCode(). - Typed exceptions: extend the existing
lib/Exception/subclasses so tests assert the class withexpectException().
Please use this issue to settle the convention (and decide what to do with the existing 1/4xx codes), then the work can be split into small PRs.
How to contribute
[!IMPORTANT]
Do not solve this issue in one PR.
Like #8053, each PR should cover one or two related source/test pairs, reference this issue without Fixes/Closes, and keep the diff small. Please comment here with the files you want to work on before starting.
Suggested order:
- Tests that assert translated messages (
$l10n->t(...)inlib/, realIL10Nin the test), because they break on any wording change. - Tests that assert literal messages in business rules (signing, certificates, policies, permissions, validation, identity flows).
- The remaining message assertions.
Do not change production behavior (messages shown to users, API payloads) only to make a test easier; when a code or a class is added, keep the message as is.
Effort
The complete issue is medium-to-high effort because it covers many files, but each PR should be low effort.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the discussion in #8092, then inspect the exception subclasses in lib/Exception/, message assertions across the PHP test files, and the code handling in ErrorPayloadBuilder and SignFileService. First document a convention for error codes or typed exceptions, including existing codes; done means the decision supports small source/test pair PRs without changing user-facing messages or API behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- php
- Domain
- backend-api-design, testing
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100