-
Notifications
You must be signed in to change notification settings - Fork 47
Upgrade Doc Detective and fix test timing issues #1495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for redpanda-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis pull request updates test configuration and automation selectors across documentation testing files. Changes include updating the doc-detective dependency from version 3.1.0 to 3.6.0, migrating element selectors from CSS-based and direct text attributes to test ID-based attributes (e.g., auth-username-input, auth-submit), adjusting wait timeouts and screenshot comparison parameters (maxVariation), changing the browser configuration from Firefox to Chrome, and reordering test steps to align with the new selector approach in both the quick-start and authentication documentation tests. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
modules/get-started/pages/quick-start.adoc (2)
238-242: Documentation tab step uses very long 15000ms wait; clarify if this is a workaround for animation/rendering delay.The 15-second wait on line 241 (after clicking the Documentation tab at line 240) is substantially longer than other waits in the flow. Per the PR objectives, this appears to be a workaround for timing issues. Verify that this delay is necessary and document the underlying cause (animation duration, data loading, browser rendering) so it can be revisited when upstream doc-detective issues (#159, #164, #165) are resolved.
Consider adding an inline comment explaining the 15000ms wait duration and linking to the related upstream doc-detective issues mentioned in the PR summary.
1-687: Overall test file shows partial migration to doc-detective v3.6.0 patterns with strategic timing workarounds.The quick-start test file demonstrates the intended migration (elementTestId selectors in login flow) and timing workarounds (2000–15000ms waits), consistent with the PR objectives. However, the file exhibits selector format inconsistency: the login flow (lines 180–182) uses the new
elementTestIdapproach, while other sections (topic filter, security) continue using legacy CSS selectors with data-testid. Given that tests currently pass (77/83 steps per PR), consider documenting whether legacy selectors are intentionally retained due to missing test IDs in targeted elements or should be systematically migrated in a follow-up refactor.Consider creating an internal tracking note or issue to systematically migrate remaining CSS selectors to
elementTestIdformat once all UI elements have test IDs assigned, to improve maintainability and alignment with doc-detective v3.6.0 best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (14)
modules/console/images/audit-logs.pngis excluded by!**/*.pngmodules/console/images/broker-overview.pngis excluded by!**/*.pngmodules/console/images/degraded-cluster.pngis excluded by!**/*.pngmodules/console/images/internal-topics.pngis excluded by!**/*.pngmodules/console/images/js-filter.pngis excluded by!**/*.pngmodules/console/images/license.pngis excluded by!**/*.pngmodules/console/images/overview.pngis excluded by!**/*.pngmodules/console/images/schema-reg.pngis excluded by!**/*.pngmodules/console/images/security.pngis excluded by!**/*.pngmodules/console/images/topic-documentation.pngis excluded by!**/*.pngmodules/console/images/topic.pngis excluded by!**/*.pngmodules/console/images/transforms.pngis excluded by!**/*.pngmodules/console/images/user.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
modules/console/pages/config/security/authentication.adoc(1 hunks)modules/get-started/pages/quick-start.adoc(8 hunks)package.json(1 hunks)tests/setup-tests/.doc-detective.json(1 hunks)tests/setup-tests/cloud/cloud-log-in.json(2 hunks)
🔇 Additional comments (6)
tests/setup-tests/.doc-detective.json (1)
11-11: Browser switch from Firefox to Chrome aligns with doc-detective v3.6.0 upgrade.This change supports the updated test infrastructure. Ensure any browser-specific behavior (rendering, timing, element discovery) has been validated, especially given the timing workarounds added elsewhere in this PR.
tests/setup-tests/cloud/cloud-log-in.json (1)
15-17: Selector migration to elementAttribute is consistent with doc-detective v3.6.0.The changes migrate from CSS selectors to semantic attribute-based targeting (name, data-provider, type), which aligns with the broader PR migration pattern and should improve selector robustness.
Also applies to: 29-31, 39-41
modules/get-started/pages/quick-start.adoc (2)
189-191: Line 189 lackselementTestIdwhile similar lines include it.Line 189 uses only
selectorandelementTestId, missing theelementTestIdfield that is present in lines 181–182. Line 180 uses onlyelementTestId(most direct), while lines 181–182 combine bothselectorandelementTestId. If the "View" button has a test ID available, line 189 should be updated to includeelementTestIdfor consistency and robustness; otherwise, document why this selector cannot be migrated.
283-283: Line 283's timeout of 100000ms is consistent with non-UI operations in the file and not unusual; however, line 289 has a structural JSON error with the "variables" key outside the "find" object.Line 283 uses a 100-second timeout that matches similar timeouts for time-intensive operations elsewhere in the file (e.g., line 136's docker pull, line 137's docker compose up). This is reasonable for operations known to be slow and is not "dramatically higher" than precedent in the file.
However, line 289 contains a malformed JSON structure where the "variables" key appears at the same level as "find" rather than nested within it. This breaks the expected step object structure. Correct the syntax to place "variables" appropriately within the step definition.
Likely an incorrect or invalid review comment.
modules/console/pages/config/security/authentication.adoc (1)
18-18: No action required — maxVariation value is correct and consistent.The
maxVariationparameter uses a 0–1 scale (not a percentage), where the default is 0.05. The value 0.10 is valid, within specification, and already standardized across all screenshot steps in the documentation. This setting allows up to 10% visual variation when comparing captured images to references, which is more permissive than the default 0.05 and will not cause test flakiness.package.json (1)
24-24: Verify doc-detective v3.6.0 compatibility against internal PR documentation.Version 3.6.0 is published and exists on npm with dependencies on doc-detective-core and doc-detective-common at matching version. However, public release notes for v3.6.0 are not available to confirm the specific fixes for data-testid parsing and XML attribute issues mentioned in the PR. The caret constraint (^3.6.0) is appropriate for safe future patches. Verify the claimed bug fixes and any breaking changes directly against internal PR documentation or release notes.
Description
This PR upgrades Doc Detective to version 3.6.0 and implements workarounds for timing issues that cause test failures when finding UI elements after navigation.
Upgraded Doc Detective
This upgrade happened in stages due to issues discovered and reported:
Initial upgrade to v3.5.0
detectStepsconfig option was being ignored (#159)data-testidattributes were being parsed incorrectlyUpgrade to v3.5.1
detectStepsbugdata-testidselector parsingFinal upgrade to v3.6.0
data-testidselectorsRelated upstream fixes:
data-testidparsing issue was fixed in the v3.6.0 releaseUpdated dependencies:
package.json- Doc Detective versionpackage-lock.json- All related dependenciesAdded navigation and screenshot steps
Added explicit
goToand screenshot steps to ensure proper page rendering before element lookups:Security Section (
modules/get-started/pages/quick-start.adoc:277-283)goTostep to navigate to/security/usersTopics Section (
modules/get-started/pages/quick-start.adoc:419-426)goTostep to navigate to/topicsImproved test reliability
Added additional
waitsteps in several locations to improve test stability:Issues encountered
Upgrade issues (Resolved in v3.6.0)
During the upgrade process, we discovered and reported critical bugs that prevented tests from running:
detectStepsconfig ignored (#159)data-testidselector parsing brokendata-testidattributes were being incorrectly parsedRemaining timing issues (workaround required)
Even with v3.6.0's improved dynamic page load resolution, we discovered timing issues where Doc Detective fails to find elements even after using
goTo+waitsteps. The elements are present in the DOM with correct selectors, but thefindstep times out.Root cause: The screenshot step appears to trigger browser rendering/paint cycles that make elements discoverable, suggesting Doc Detective may not be waiting for the page to be fully interactive after navigation, despite the v3.5.0 improvements to network traffic monitoring.
Workaround: Adding a
screenshotstep before problematicfindoperations resolves the issue. While not ideal, this workaround:Related upstream issues reported:
maxVariationIs interpreted as percent instead of fraction doc-detective/doc-detective#164 (Navigation reliability)Testing
All tests now pass successfully:
Results:
Future work
Once the upstream Doc Detective issues are resolved, we can:
Page previews
Checks