fix: use Page Object pattern for flaky Geb tests across all subprojects#15410
Open
jamesfredley wants to merge 8 commits intoapache:7.0.xfrom
Open
fix: use Page Object pattern for flaky Geb tests across all subprojects#15410jamesfredley wants to merge 8 commits intoapache:7.0.xfrom
jamesfredley wants to merge 8 commits intoapache:7.0.xfrom
Conversation
Refactor all Geb functional tests to use the Page Object pattern established in PR apache#15394, replacing direct go() navigation and bare title assertions with to(PageObject)/at(PageObject) calls. Changes across 8 subprojects (app1, app2, app3, exploded, geb, hibernate5, mongodb, namespaces) and scaffolding-fields: - Create 28 new Page Object classes with static url, pageTitle, and at checks following the PR apache#15394 convention - Replace go('/url') with to(PageObject) for initial page navigation - Replace bare title == 'X' in then: blocks with at(PageObject) - Use waitFor { title == PageObject.pageTitle } for post-click async page transitions only - Keep go() with waitFor for special cases (context paths, interceptor routing, parameterized URLs) - Update grails-geb FunctionalSpec template with waitFor for title checks since templates cannot reference project-specific Page Objects - Revert unnecessary waitFor additions to geb subproject specs that already used to(PageObject) correctly Assisted-by: Claude Code <Claude@Claude.ai>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request extends the Geb Page Object pattern from PR #15394 to eliminate flaky test assertions across 8 subprojects in the repository. The PR aims to stabilize Geb functional tests by replacing direct go() + bare title == assertions with the Page Object pattern using to() and at() methods.
Changes:
- Created 28 new Page Object classes following a consistent structure with
pageTitle,url, andatproperties - Refactored 24 spec files to use Page Objects for navigation and assertions
- Removed debug println statements and unused imports (PendingFeature)
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-test-examples/scaffolding-fields/src/integration-test/groovy/scaffoldingfields/FieldTypesSpec.groovy | Refactored to use EmployeeCreatePage for setup navigation |
| grails-test-examples/scaffolding-fields/src/integration-test/groovy/scaffoldingfields/CrudFunctionalSpec.groovy | Refactored CRUD test navigation to use Page Objects with proper waitFor for async transitions |
| grails-test-examples/namespaces/.../FrontendPagePage.groovy | New Page Object for frontend page (naming issue: redundant "Page") |
| grails-test-examples/namespaces/.../AdminPagePage.groovy | New Page Object for admin page (naming issue: redundant "Page") |
| grails-test-examples/namespaces/.../ReportPage.groovy | New Page Object for admin report (non-namespaced) |
| grails-test-examples/namespaces/.../AdminReportPage.groovy | New Page Object for admin report (namespaced) |
| grails-test-examples/namespaces/.../ReportControllerSpec.groovy | Refactored to use Page Objects for navigation |
| grails-test-examples/namespaces/.../PageControllerSpec.groovy | Refactored to use Page Objects, removed unused import |
| grails-test-examples/namespaces/.../ContextPathSpec.groovy | Correctly kept go() with waitFor for context path testing |
| grails-test-examples/mongodb/hibernate5/.../BookShowPage.groovy | New Page Object for book show page |
| grails-test-examples/mongodb/hibernate5/.../BookListPage.groovy | New Page Object for book list page |
| grails-test-examples/mongodb/hibernate5/.../AuthorShowPage.groovy | New Page Object for author show page |
| grails-test-examples/mongodb/hibernate5/.../AuthorListPage.groovy | New Page Object for author list page |
| grails-test-examples/mongodb/hibernate5/.../BookControllerSpec.groovy | Refactored to use Page Objects with waitFor for async save |
| grails-test-examples/mongodb/hibernate5/.../AuthorControllerSpec.groovy | Refactored to use Page Objects with waitFor for async save |
| grails-test-examples/mongodb/base/.../BookShowPage.groovy | New Page Object for book show page |
| grails-test-examples/mongodb/base/.../BookListPage.groovy | New Page Object for book list page |
| grails-test-examples/mongodb/base/.../BookControllerSpec.groovy | Refactored to use Page Objects with waitFor for async save |
| grails-test-examples/hibernate5/grails-hibernate/.../BookShowPage.groovy | New Page Object for book show page |
| grails-test-examples/hibernate5/grails-hibernate/.../BookListPage.groovy | New Page Object for book list page |
| grails-test-examples/hibernate5/grails-hibernate/.../BookControllerSpec.groovy | Refactored to use Page Objects with waitFor for async save |
| grails-test-examples/exploded/.../LoginAuthPage.groovy | New Page Object for login page |
| grails-test-examples/exploded/.../LoadAfterSpec.groovy | Refactored to use LoginAuthPage |
| grails-test-examples/app3/.../LoginAuthPage.groovy | New Page Object for login page |
| grails-test-examples/app3/.../LoadAfterSpec.groovy | Refactored to use LoginAuthPage |
| grails-test-examples/app2/.../PageNotFoundPage.groovy | New Page Object for 404 page |
| grails-test-examples/app2/.../FooListPage.groovy | New Page Object for foo list with query param |
| grails-test-examples/app2/.../NotFoundHandlerSpec.groovy | Special case: correctly uses go() with waitFor and Page Object constants |
| grails-test-examples/app1/.../FooFunctionalSpec.groovy | Refactored to use FooListPage |
| grails-test-examples/app1/.../BarFunctionalSpec.groovy | Refactored to use BarListPage |
| grails-test-examples/app1/.../PartialPage.groovy | New Page Object for partial template page |
| grails-test-examples/app1/.../LoginAuthPage.groovy | New Page Object for login page |
| grails-test-examples/app1/.../HomePage.groovy | New Page Object for home page |
| grails-test-examples/app1/.../FooListPage.groovy | New Page Object for foo list |
| grails-test-examples/app1/.../FooLayoutSnippetPage.groovy | New Page Object for layout snippet |
| grails-test-examples/app1/.../FooLayoutPage.groovy | New Page Object for layout page |
| grails-test-examples/app1/.../ConventionLayoutPage.groovy | New Page Object for convention layout |
| grails-test-examples/app1/.../BookShowPage.groovy | New Page Object for book show page |
| grails-test-examples/app1/.../BookListPage.groovy | New Page Object for book list page |
| grails-test-examples/app1/.../BookCreatePage.groovy | New Page Object with content definitions for form elements |
| grails-test-examples/app1/.../BarListPage.groovy | New Page Object for bar list |
| grails-test-examples/app1/.../ActionReturnsNullPage.groovy | New Page Object for action returns null test |
| grails-test-examples/app1/.../LayoutWithTemplateSpec.groovy | Refactored to use PartialPage |
| grails-test-examples/app1/.../LayoutFunctionalSpec.groovy | Refactored to use layout Page Objects |
| grails-test-examples/app1/.../MiscFunctionalSpec.groovy | Refactored to use ActionReturnsNullPage |
| grails-test-examples/app1/.../LoadAfterSpec.groovy | Refactored to use LoginAuthPage |
| grails-test-examples/app1/.../HomeSpec.groovy | Refactored to use HomePage, removed debug code |
| grails-test-examples/app1/.../BookFunctionalSpec.groovy | Refactored to use Page Objects, removed debug code, critical bug with parameterized navigation |
| grails-geb/src/main/templates/FunctionalSpec.groovy | Updated template to use waitFor pattern for generated tests |
Comments suppressed due to low confidence (2)
grails-test-examples/scaffolding-fields/src/integration-test/groovy/scaffoldingfields/CrudFunctionalSpec.groovy:237
- For consistency with other waitFor statements in this file and to avoid hardcoding the title, consider using
waitFor { title == EmployeeCreatePage.pageTitle }instead of the hardcoded string 'Create Employee'.
grails-test-examples/app1/src/integration-test/groovy/functionaltests/pages/HomePage.groovy:27 - The static url property should have a leading slash to match the established codebase convention. Change to url = '' for the root page (note: this is already an empty string, but existing HomePage in geb uses '/' for clarity).
static url = ''
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
grails-test-examples/app1/src/integration-test/groovy/functionaltests/pages/BookListPage.groovy
Outdated
Show resolved
Hide resolved
grails-test-examples/app2/src/integration-test/groovy/app2/pages/FooListPage.groovy
Outdated
Show resolved
Hide resolved
grails-test-examples/app1/src/integration-test/groovy/functionaltests/BookFunctionalSpec.groovy
Show resolved
Hide resolved
...s-test-examples/namespaces/src/integration-test/groovy/namespaces/pages/AdminPagePage.groovy
Outdated
Show resolved
Hide resolved
grails-test-examples/app3/src/integration-test/groovy/app3/pages/LoginAuthPage.groovy
Outdated
Show resolved
Hide resolved
...es/mongodb/hibernate5/src/integration-test/groovy/functional/tests/pages/BookListPage.groovy
Outdated
Show resolved
Hide resolved
...s-test-examples/namespaces/src/integration-test/groovy/namespaces/pages/AdminPagePage.groovy
Outdated
Show resolved
Hide resolved
grails-test-examples/app1/src/integration-test/groovy/functionaltests/pages/BookShowPage.groovy
Outdated
Show resolved
Hide resolved
...ls-test-examples/app1/src/integration-test/groovy/functionaltests/pages/FooLayoutPage.groovy
Outdated
Show resolved
Hide resolved
...est-examples/namespaces/src/integration-test/groovy/namespaces/pages/FrontendPagePage.groovy
Outdated
Show resolved
Hide resolved
…minPagePage/FrontendPagePage - Add leading slash to static url in all 28 new Page Objects for consistency with existing geb and scaffolding-fields conventions - Rename AdminPagePage to AdminPage and FrontendPagePage to FrontendPage to eliminate redundant 'Page' suffix - Update PageControllerSpec imports and usages for renamed classes Assisted-by: Claude Code <Claude@Claude.ai>
Contributor
|
Assuming @matrei is ok with these changes, I'm ok to merge. |
jamesfredley
force-pushed
the
fix/flaky-geb-tests
branch
from
f6af480 to
4e643d2
Compare
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
3 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.
Summary
Extends the Page Object pattern from #15394 to all remaining Geb functional tests across the repository, eliminating flaky bare
title == 'X'assertions that fail intermittently due to race conditions betweendriver.get()and page title availability.Changes
28 new Page Object classes created across 8 subprojects following the exact convention from #15394:
static String pageTitle- page title constantstatic url- relative URL pathstatic at = { title == pageTitle }- at-check closurestatic content- only when needed (e.g., form elements)24 spec files refactored:
go('/url')replaced withto(PageObject)for initial navigationtitle == 'X'inthen:blocks replaced withat(PageObject)waitFor { title == PageObject.pageTitle }used only for post-click async transitionsgo()+waitForretained for special cases (context paths, interceptor routing, parameterized URLs)Subprojects affected: app1, app2, app3, exploded, geb, hibernate5, mongodb (base + hibernate5), namespaces, scaffolding-fields, grails-geb (template)
Special Cases
go()withwaitForand Page ObjectpageTitlereferencesgo()withwaitForwaitFordirectlyto(PageObject)correctly - reverted unnecessarywaitForadditionsGebSpec- skippedVerification
./gradlew codeStylepasses (all CodeNarc checks clean)