Add artefact name contract characterization spec - #15998
Conversation
Assisted-by: opencode:gpt-5.6-sol
There was a problem hiding this comment.
Pull request overview
Adds a new Spock characterization spec to lock in the current GrailsClass artefact naming outputs (controllers/services/domains/urlMappings) and stability across repeated accessor reads and wrapper re-construction, to protect future GrailsNameUtils precomputation work.
Changes:
- Introduce
ArtefactNamePrecomputationSpecto assert deterministic naming fields (name,shortName,fullName,packageName,propertyName,logicalPropertyName,naturalName) for representative artefact classes. - Add repeated-read and repeated-wrapper-construction checks to ensure naming accessors remain stable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| packageName: BASE_PACKAGE, | ||
| propertyName: 'HTMLController', | ||
| logicalPropertyName: 'HTML', | ||
| naturalName: 'HTMLC ontroller' | ||
| ], |
| packageName: BASE_PACKAGE, | ||
| propertyName: 'JSONAPIService', | ||
| logicalPropertyName: 'JSONAPI', | ||
| naturalName: 'JSONAPIS ervice' | ||
| ], |
| packageName: BASE_PACKAGE, | ||
| propertyName: 'URLDomain', | ||
| logicalPropertyName: 'URLDomain', | ||
| naturalName: 'URLD omain' | ||
| ], |
| packageName: BASE_PACKAGE, | ||
| propertyName: 'HTMLController', | ||
| logicalPropertyName: 'HTML', | ||
| naturalName: 'HTMLC ontroller' | ||
| ], |
| packageName: BASE_PACKAGE, | ||
| propertyName: 'JSONAPIService', | ||
| logicalPropertyName: 'JSONAPI', | ||
| naturalName: 'JSONAPIS ervice' | ||
| ] |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #15998 +/- ##
==================================================
+ Coverage 52.3514% 52.3618% +0.0104%
- Complexity 18299 18305 +6
==================================================
Files 2036 2036
Lines 96347 96347
Branches 16829 16829
==================================================
+ Hits 50439 50449 +10
+ Misses 38485 38475 -10
Partials 7423 7423 🚀 New features to boost your workflow:
|
| ] | ||
| } | ||
|
|
||
| private GrailsClass grailsClassFor(String artifactType, Class<?> wrapperClass) { |
There was a problem hiding this comment.
Constructing DefaultGrailsControllerClass/DefaultGrailsServiceClass/etc. directly bypasses artefact detection entirely — these constructors accept any class (new DefaultGrailsControllerClass(URLDomain) would work just as well). So this spec pins the naming metadata contract, but not the detection contract (ArtefactHandler.isArtefact), which is the part a precomputation refactor is most likely to disturb.
Consider adding cases that exercise the handlers, e.g.:
- an abstract
FooControlleris rejected (ControllerArtefactHandlerpassesallowAbstract=false) URLDomainwithout@Entity/@Artefactis rejected byDomainClassArtefactHandler.isArtefactClass(domain detection is annotation/location-based; the name plays no role)- a suffix match like
HTMLControlleris accepted byControllerArtefactHandler.isArtefactClass
| /** | ||
| * Tests for deterministic GrailsClass naming metadata. | ||
| */ | ||
| class ArtefactNamePrecomputationSpec extends Specification { |
There was a problem hiding this comment.
"Precomputation" in the spec name is aspirational — nothing here asserts precomputation; the repeated-read/repeated-construction tests pin stability only. That's a fine pre-refactor baseline, but consider a name that says what it locks in, e.g. ArtefactNamingContractSpec, so it doesn't read as if precomputed behavior already exists.
| packageName: BASE_PACKAGE, | ||
| propertyName: 'HTMLController', | ||
| logicalPropertyName: 'HTML', | ||
| naturalName: 'HTMLC ontroller' |
There was a problem hiding this comment.
For the record: expectations like 'HTMLC ontroller' / 'JSONAPIS ervice' / 'URLD omain' are correct characterizations — GrailsNameUtils.getNaturalName genuinely splits acronym-prefixed names after the last uppercase letter. Worth a brief code comment noting these pin known-quirky behavior intentionally, so a future reader doesn't "fix" the expectations (or the algorithm) without realizing this spec exists to catch exactly that change.
…n spec Copilot's 5 inline comments claiming the acronym naturalName expectations were wrong (e.g. 'HTMLC ontroller') are false positives - verified by running the spec: GrailsNameUtils.getNaturalName genuinely produces those values, and jdaugherty's review already confirmed this. No change needed there. jdaugherty's own feedback was substantive and is addressed here: - Rename ArtefactNamePrecomputationSpec -> ArtefactNamingContractSpec: "precomputed" was aspirational, since nothing in the spec exercises actual precomputation, only naming stability. - Add a comment above the acronym-heavy naturalName assertions explaining the quirky-but-intentional GrailsNameUtils splitting behavior they pin, so a future reader doesn't "fix" the expectations or the algorithm without realizing this spec exists to catch exactly that change. - Add three cases exercising the artefact detection contract (ArtefactHandler#isArtefactClass), which the original spec bypassed entirely by constructing GrailsClass wrappers directly: an abstract controller is rejected (ControllerArtefactHandler's allowAbstract is false), a suffix-matching concrete controller is accepted, and a domain-named class with no @Entity/@ArteFact annotation is rejected by DomainClassArtefactHandler - this is the part a naming precomputation refactor is most likely to disturb, and the prior spec gave it no coverage at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
✅ All tests passed ✅🏷️ Commit: 603f812 Learn more about TestLens at testlens.app. |
Summary
Verification
This is an AI-generated starting point that pins the naming contract ahead of any GrailsNameUtils precomputation work.