fix: update assertion method in DebuggerHelper to use GetNAssertElementText #40937
Conversation
…ntText Why was this updated? The assertion for when the count is equal to zero would always pass regardless of the actual error count. For example, if there are three errors, the text in the error count would be "debug space three." The previous assertion used "assert contains," which would always include the text "debug," making it true regardless of the number of errors visible in the console. I have now replaced it with "get assert element text," which enforces strict checking for an empty debug message when the error count is zero.
WalkthroughThe internal logic of the Changes
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/cypress/support/Pages/DebuggerHelper.ts (1)
144-145: Locator duplication can be consolidatedBoth
_debuggerIcon(line 16) and_errorCount(line 24) resolve to the same selector.t--debugger-count. This divergence in naming invites accidental drift.Consider removing
_errorCountand re-using_debuggerIcon, or vice-versa, to avoid duplicated selectors and keep maintenance overhead low.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/cypress/support/Pages/DebuggerHelper.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test code written using the Cypress test library. Ensure that: - Follow best practices for Cypress code and e2e automation. ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/Pages/DebuggerHelper.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: mark-stale
- GitHub Check: build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
| const assertion = count > 0 ? `Debug (${count})` : "Debug "; | ||
| this.agHelper.GetNAssertElementText(this.locators._errorCount, assertion); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid brittle equality check caused by trailing space
Relying on the literal string "Debug " (note the trailing space) makes the assertion brittle—any UI trim, refactor, or font-based non-breaking space will break the test.
Prefer a whitespace-insensitive comparison, e.g. assertion.trim() or a regex that tolerates optional whitespace.
-const assertion = count > 0 ? `Debug (${count})` : "Debug ";
-this.agHelper.GetNAssertElementText(this.locators._errorCount, assertion);
+const assertion =
+ count > 0 ? `Debug (${count})` : "Debug"; // no trailing space
+this.agHelper.GetNAssertElementText(
+ this.locators._errorCount,
+ assertion,
+ { trim: true }, // assume helper supports an options bag
+);If GetNAssertElementText lacks a trim/regex option, consider switching to a helper that does, or wrap the retrieved text with .trim() before comparing.
This small tweak will harden the test against UI whitespace changes.
🏁 Script executed:
#!/bin/bash
# Locate the definition of GetNAssertElementText to inspect its signature and default behavior
rg -n "GetNAssertElementText" -n app/client/cypress/supportLength of output: 693
🏁 Script executed:
#!/bin/bash
# Show definition and surrounding lines for GetNAssertElementText
rg -n "public GetNAssertElementText" -n app/client/cypress/support/Pages/AggregateHelper.ts
sed -n '360,420p' app/client/cypress/support/Pages/AggregateHelper.tsLength of output: 2058
Avoid brittle equality check caused by trailing space
Using the literal "Debug " (with a trailing space) makes this assertion fragile—any UI trim or accidental whitespace change will break the test. Remove the trailing space and switch to a containment assertion:
- const assertion = count > 0 ? `Debug (${count})` : "Debug ";
- this.agHelper.GetNAssertElementText(this.locators._errorCount, assertion);
+ const assertion = count > 0 ? `Debug (${count})` : "Debug";
+ this.agHelper.GetNAssertElementText(
+ this.locators._errorCount,
+ assertion,
+ "contain.text", // tolerates surrounding whitespace
+ );If you require an exact match, you can invoke .invoke("text").then(text => expect(text.trim()).to.equal(assertion)) or extend GetNAssertElementText to accept a trim option. This change hardens the test against incidental whitespace.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/client/cypress/support/Pages/DebuggerHelper.ts around lines 144-145, the
assertion uses a literal string with a trailing space ("Debug "), making the
test fragile to whitespace changes. Remove the trailing space from the string
and modify the assertion to either trim the retrieved text before comparison or
use a containment check instead of strict equality. If GetNAssertElementText
does not support trimming, replace it with a custom assertion that invokes
.invoke("text") on the element and compares the trimmed text to the expected
string to make the test resilient to incidental whitespace.
Description
Problem
The existing assertion in
DebuggerHelperfalsely passed even when there were visible errors in the debug console, leading to unreliable test validations.Root cause
The assertion used
assert contains, which only checked for the presence of the word "debug". Since this word is present regardless of the actual error count, the assertion would always pass, even if there were errors.Solution
This PR handles the update of the assertion method in
DebuggerHelperto usegetNAssertElementTextinstead ofassert contains. This enforces strict validation by checking for an exact match in the debug message text, ensuring accurate test outcomes when error count is expected to be zero.Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Datasource, @tag.Widget, @tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15633758023
Commit: 105857f
Cypress dashboard.
Tags:
@tag.Datasource, @tag.Widget, @tag.SanitySpec:
Fri, 13 Jun 2025 13:52:55 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit