Skip to content

Remove jodd-wot from fields tests - #15965

Open
jamesfredley wants to merge 2 commits into
8.0.xfrom
chore/remove-jodd-wot
Open

Remove jodd-wot from fields tests#15965
jamesfredley wants to merge 2 commits into
8.0.xfrom
chore/remove-jodd-wot

Conversation

@jamesfredley

Copy link
Copy Markdown
Contributor

Summary

Removes the jodd-wot test dependency from grails-fields by replacing its only usage with direct Spock string assertions against the rendered field output.

Changes

Area Change
grails-fields tests Removed jodd.lagarto.dom.jerry.Jerry imports and helper usage in DefaultFieldTemplateSpec.
grails-fields dependencies Removed org.jodd:jodd-wot from test dependencies.
Root properties Removed the now-unused joddWotVersion property.

Verification

Check Result
`rg -n "jodd jodd-wot"`
./gradlew.bat :grails-fields:test --tests "grails.plugin.formfields.DefaultFieldTemplateSpec" Passed.
./gradlew.bat :grails-fields:test Passed.

Checklist

  • Searched for jodd-wot / jodd references.
  • Replaced test usage with plain Spock assertions.
  • Removed unused dependency/version wiring.
  • Verified affected module tests pass.

Replace the Jerry-based spec assertions with direct output checks and remove the unused jodd-wot test dependency wiring.

Assisted-by: opencode:gpt-5.5

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the jodd-wot test dependency from the grails-fields module by replacing the prior DOM-parsing assertions in DefaultFieldTemplateSpec with direct string-based Spock assertions, and then cleaning up the related Gradle dependency and version property wiring.

Changes:

  • Refactored DefaultFieldTemplateSpec to drop Jerry/jerry usage and assert against rendered output strings.
  • Removed org.jodd:jodd-wot from grails-fields test dependencies.
  • Removed the now-unused joddWotVersion property from gradle.properties.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
grails-fields/src/test/groovy/grails/plugin/formfields/DefaultFieldTemplateSpec.groovy Removes Lagarto/Jerry usage and replaces DOM assertions with string assertions for default/invalid/required rendering.
grails-fields/build.gradle Drops the jodd-wot test dependency block (and its excludes).
gradle.properties Removes the unused joddWotVersion property.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.4835%. Comparing base (c317eef) to head (3b8b35f).
⚠️ Report is 264 commits behind head on 8.0.x.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #15965        +/-   ##
==================================================
+ Coverage     49.5558%   51.4835%   +1.9277%     
- Complexity      16930      17777       +847     
==================================================
  Files            1999       2039        +40     
  Lines           93753      95586      +1833     
  Branches        16419      16591       +172     
==================================================
+ Hits            46460      49211      +2751     
+ Misses          40131      39064      -1067     
- Partials         7162       7311       +149     

see 238 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jdaugherty

Copy link
Copy Markdown
Contributor

I'm not a fan of the string comparisons - they're fragile and brittle. Why are we not using a standard html parser?

@jamesfredley

Copy link
Copy Markdown
Contributor Author

Fair point on the brittleness. Worth separating the goal from the assertion style:

The primary objective (pre-release review, Codebase 3 / hygiene quick-win) is to remove the jodd-wot dependency - jodd-wot 3.3.8 dates to 2012 and is effectively unmaintained, and it's a single testImplementation usage in grails-fields/build.gradle. Getting it off the dependency list is the win.

On how to replace the assertion, the review lists two options: "modern assertions" or "Jsoup or equivalent". The trade-off:

  • String assertions (current PR): removes jodd-wot with zero new dependencies, but is more brittle as you note.
  • Jsoup: robust HTML parsing, but re-adds a test dependency to replace the one we're removing.

I'm happy to switch these to Jsoup (org.jsoup:jsoup, testImplementation) if you'd rather have a real parser and consider the added test dep acceptable - that still achieves the jodd-wot removal, just trading one modern dep for one archived one. Which would you prefer: keep it dependency-free with tightened assertions, or move to Jsoup?

@jdaugherty

Copy link
Copy Markdown
Contributor

I'm happy with Jsoup, it's a parser that's well understood. I think we need to parse these values though, especially with some of the refactoring we've talked about. Otherwise, the tests are too brittle.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
jdaugherty flagged the string-based assertions this PR introduced as
fragile (exact-serialization matching instead of semantic checks), and
Copilot independently raised the same concern per-assertion. jamesfredley
proposed the trade-off directly: stay dependency-free with tightened
(regex) assertions, or trade the removed jodd-wot for a real, modern
parser (Jsoup). jdaugherty chose Jsoup explicitly, anticipating more
HTML-structure-sensitive tests from planned refactoring.

Reimplements the three DefaultFieldTemplateSpec assertions against
org.jsoup instead of raw strings, restoring the exact semantic checks
the original jodd-wot/Jerry version had (root div has the fieldcontain/
error/required class tokens - order and other-attributes insensitive -
label text/for-attribute, label immediately precedes the input, and the
required-indicator span's text) rather than the newly-added brittle
literal-markup comparisons.

jsoup has no existing version management in this repo; added
jsoupVersion to gradle.properties following the same pattern already
used for javassistVersion/jnrPosixVersion, since it's a single-module,
test-only dependency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 27, 2026
@testlens-app

testlens-app Bot commented Jul 27, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 3b8b35f
▶️ Tests: 16025 executed
⚪️ Checks: 59/59 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants