Copilot commented on code in PR #15394:
URL: https://github.com/apache/grails-core/pull/15394#discussion_r2817614994
##########
grails-test-examples/scaffolding/src/integrationTest/groovy/com/example/UserCommunityControllerSpec.groovy:
##########
@@ -16,43 +16,31 @@
* specific language governing permissions and limitations
* under the License.
*/
-
package com.example
import com.example.pages.LoginPage
+import com.example.pages.LogoutPage
+import com.example.pages.CommunityUserListPage
import grails.plugin.geb.ContainerGebSpec
import grails.testing.mixin.integration.Integration
-@Integration(applicationClass = Application)
+@Integration
class UserCommunityControllerSpec extends ContainerGebSpec {
void setup() {
- go '/'
- to LoginPage
- username = '[email protected]'
- password = 'letmein'
- loginButton.click()
+ to(LoginPage).login()
}
void cleanup() {
- try {
- go 'logout'
- $('input', value: 'Log Out').click()
- }
- catch (ignored) {
- // ignored
- }
+ to(LogoutPage).logout()
Review Comment:
`cleanup()` no longer ignores logout failures; it always does
`to(LogoutPage).logout()`. If the session is already unauthenticated (or
`setup()` failed), `/logout` may redirect to the login page and the
`LogoutPage.at` check will fail during teardown, turning unrelated issues into
spec failures. Consider adding a guard (e.g., skip when at `LoginPage`) or
restoring a try/catch around logout to keep cleanup resilient.
```suggestion
try {
to(LogoutPage).logout()
} catch (Exception ignored) {
// Ignore logout failures to keep cleanup resilient
}
```
##########
grails-test-examples/scaffolding/src/integrationTest/groovy/com/example/UserControllerSpec.groovy:
##########
@@ -16,43 +16,31 @@
* specific language governing permissions and limitations
* under the License.
*/
-
package com.example
import com.example.pages.LoginPage
+import com.example.pages.LogoutPage
+import com.example.pages.UserListPage
import grails.plugin.geb.ContainerGebSpec
import grails.testing.mixin.integration.Integration
-@Integration(applicationClass = Application)
+@Integration
class UserControllerSpec extends ContainerGebSpec {
void setup() {
- go '/'
- to LoginPage
- username = '[email protected]'
- password = 'letmein'
- loginButton.click()
+ to(LoginPage).login()
}
void cleanup() {
- try {
- go 'logout'
- $('input', value: 'Log Out').click()
- }
- catch (ignored) {
- // ignored
- }
+ to(LogoutPage).logout()
Review Comment:
`cleanup()` used to swallow logout/navigation errors, but it now
unconditionally navigates to `/logout` and performs an `at` check via
`to(LogoutPage)`. If the session is already logged out (or login failed in
`setup()`), `/logout` can redirect back to the login page and cause teardown to
fail the spec for an unrelated reason. Consider reintroducing a guard (e.g.,
only attempt logout when not at `LoginPage`, or wrap logout in a try/catch) so
cleanup doesn’t introduce new flakiness or mask the original failure cause.
```suggestion
try {
// Only attempt logout if we're not already on the login page.
if (!(page instanceof LoginPage)) {
to(LogoutPage).logout()
}
} catch (Exception ignored) {
// Swallow cleanup errors to avoid masking the original test
failure.
}
```
##########
grails-test-examples/scaffolding/src/integrationTest/groovy/com/example/pages/LogoutPage.groovy:
##########
@@ -16,19 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-
package com.example.pages
import geb.Page
class LogoutPage extends Page {
- static url = 'logout'
-
- static at = { title == 'Confirm Log Out?' }
+ static String pageTitle = 'Confirm Log Out?'
+ static url = 'logout'
+ static at = { title == pageTitle }
static content = {
logoutForm { $('form') }
- logoutButton { $('button', value: 'Log Out') }
+ logoutButton { $('button') }
+ }
+
+ void logout() {
+ logoutButton.click()
+ waitFor { title != pageTitle }
Review Comment:
`logoutButton { $('button') }` is very broad and can become ambiguous if the
logout confirmation page ever contains additional buttons (layout/header, JS
widgets, etc.). Since you already define `logoutForm`, consider scoping the
selector to the form and/or matching on submit semantics (type/text) so the
click is deterministic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
