Katalon Code Review

Katalon Code Review

Review Groovy code in the Katalon framework (project Katalon.UI.Tests). The framework wraps Katalon with custom abstractions — Perform.* over WebUI.*, Page.As() factory, locator() Playwright-style selectors, completeAllRequiredFields data-driven form filling. Generic Katalon advice does not apply; review against the team's specific rules below.

When to use this skill

Trigger in any of these situations:

• User shares a .groovy file from Test Cases/Clients/ or Keywords/katalon/
• User shares a PR link, diff, or git diff output touching those folders
• User asks "review", "check", "audit", "feedback", "looks good?"
• User asks "review my changes", "review my work", "review what I have", or any review request without naming a specific file — default to reviewing current git working state
• Conversation already involves Katalon and the user shares any Groovy snippet

If the file is NOT in the Katalon codebase (e.g., generic Groovy script), defer to the user — confirm whether Katalon-specific rules apply.

Procedure

1. Determine the review target — pick exactly one source:

   Signal	Target
   User pasted code inline	Review the pasted code
   User named a specific file/path	Read and review that file
   User shared a PR URL or PR number	Run gh pr diff <N> directly; only ask user to paste the diff if the command fails
   User said "review my changes today" or temporal phrase	Use git log --since='1 day ago' --name-only --pretty=format: then diff each file
   Default — no target named	Review the full PR-equivalent scope (see below)

   Default workflow — "PR-equivalent" scope:

   The goal is to review everything this PR would introduce when opened: committed-but-not-yet-merged changes on the current branch PLUS any uncommitted local changes on top.

   # 1. Detect current branch name
   git rev-parse --abbrev-ref HEAD
   
   # 2. Determine base branch — prefer the real PR target over heuristics:
   gh pr view --json baseRefName -q .baseRefName 2>/dev/null   # use this if non-empty
   # If empty, fall back to detecting from remote HEAD:
   git symbolic-ref refs/remotes/origin/HEAD 2>/dev/null | sed 's@^refs/remotes/origin/@@'
   # If still empty, probe common names in order:
   for b in main master develop; do
       git rev-parse --verify origin/$b >/dev/null 2>&1 && echo "$b" && break
   done
   

   # 3. Once base branch is known (call it BASE), fetch to avoid stale ref, then gather scope:
   git fetch origin $BASE --quiet
   git log origin/$BASE..HEAD --oneline                         # commits on this branch
   git diff origin/$BASE...HEAD                                 # committed changes vs base (3 dots = since merge-base)
   git diff HEAD                                                # uncommitted on top
   git ls-files --others --exclude-standard '*.groovy'          # untracked .groovy files
   

   Branch-state matrix — pick scope based on what's found:

   Current branch	Commits on branch	Uncommitted	Scope to review
   Feature branch	Yes (≥1)	Yes	Full PR equivalent: git diff origin/$BASE + untracked
   Feature branch	Yes (≥1)	No	Committed only: git diff origin/$BASE...HEAD
   Feature branch	No	Yes	Uncommitted only: git diff HEAD + untracked
   main/master/develop	—	Yes	Uncommitted only: git diff HEAD + untracked
   main/master/develop	—	No	Reply: No pending changes on default branch. Specify a file or check out a feature branch.
   Detached HEAD / no remote	—	—	Fallback to uncommitted only, prefix warning: Cannot detect PR scope (detached HEAD or no remote). Reviewing uncommitted changes only.

   Then for each affected file:

   • Filter to in-scope only: .groovy files inside Test Cases/, Keywords/katalon/, Scripts/, Test Listeners/. Skip generated paths (build/, bin/, .cache/, .gradle/), reports, data files.
   • New files (untracked or added on branch) → read full content
   • Modified files → focus on changed hunks + immediate context (±5 lines)
   • Deleted files → skip (nothing to review)

   Edge cases for default mode:

   • Pending changes all out-of-scope (no .groovy in scope folders) → Pending changes are outside the Katalon scope: <list of files>. Confirm if you want them reviewed anyway. Wait for user.
   • Very large scope (> 15 files or > 800 lines) → Found <N> changed files / <M> lines across <K> commits. Review all, narrow to a folder, or limit to last commit (git diff HEAD~1)?
   • Working directory is not a git repo → Not in a git repository. Please specify a file or paste code to review.
   • Base branch detection failed completely → ask user: Cannot detect default branch. Is it 'main', 'master', 'develop', or other?

2. Identify the artifact type (per file):

   • Test Cases/Clients/<Client>/... → test case mode
   • Keywords/katalon/pages/common/* → common page object mode
   • Keywords/katalon/pages/<client>/* → client page object mode
   • Keywords/katalon/core/* or common/helpers/* → framework mode (most strict)
   • PR/diff → review only changed lines + immediate context

3. Determine mode:

   • Default: pragmatic (flag 🔴 + 🟡 only)
   • Switch to strict if user wrote "strict", "thorough", or invoked /katalon-code-review strict

4. Run checks in order: hard-rule blockers → recurring-issue warnings → (strict only) suggestions

5. Format per ## Output format, do not deviate

6. End with a verdict: APPROVE / REQUEST CHANGES / COMMENT-ONLY

The 9 hard rules (🔴 Blockers)

Rule 1: Form filling MUST use completeAllRequiredFields

Why it matters: Manual field-by-field calls couple test logic to UI layout — a renamed label or added field breaks the test silently and leaves no traceability back to the data source.

Manual sequences of Perform.click / Perform.inputText / Perform.selectDropdown to fill form fields are forbidden. All form data must flow through the data-driven dispatcher.

Bad:

Perform.inputText(locator("label:text-is('First Name') + input"), "John")
Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
Perform.selectDropdown(locator("label:text-is('State') + select"), "NY")

Good:

def data = DataHelper.JSON.render("DataStorage/Seeds/<Client>/<TICKET>.json", [:])
Page.As(SomePage).completeAllRequiredFields(data.SomeScreen.Fields)

Rule 2: Page interaction MUST use Page.As()

Why it matters: Direct instantiation bypasses the factory's scope and lifecycle management, making cross-frame and client-context handling inconsistent and causing hard-to-reproduce failures.

Direct page instantiation or static factories are forbidden in test cases.

Bad:

def loginPage = new LoginPage()
loginPage.logInBySimulator(creds, data)

Good:

Page.As(LoginPage).logInBySimulator(creds, data)

Rule 3: WebUI.* ONLY when Perform.* lacks a wrapper

Why it matters: Perform.* adds retry logic, uniform failure handling, and timeout normalization. Bypassing it via raw WebUI.* skips built-in waits and produces inconsistent failure messages, causing intermittent failures that are hard to diagnose.

If Perform has the operation, use it. Falling back to raw WebUI.* is only allowed for native operations Perform doesn't wrap.

Known direct-WebUI exceptions (acceptable): WebUI.uploadFile, WebUI.switchToWindowIndex, WebUI.findWebElements, browser/window-level navigation that Perform doesn't expose.

Bad (Perform has wrapper):

WebUI.click(smartOR(btnSubmit))
WebUI.setText(smartOR(txtName), "John")
WebUI.verifyElementVisible(smartOR(lblTitle))

Good:

Perform.click(btnSubmit)
Perform.inputText(txtName, "John")
Perform.verifyElement(lblTitle, Expectation.Visible, defaultTimeout, FailureHandling.STOP_ON_FAILURE)

Rule 4: Inheritance chain — CommonPage → ClientCommonPage → SpecificPage

Why it matters: Skipping the client tier loses client-specific helpers and default configurations, forcing duplicated overrides and making page objects unable to access client context (e.g., client-specific frame handling, base URLs, shared selectors).

• Pages in pages/common/* extend CommonPage
• Client-common pages in pages/<client>/<Client>CommonPage.groovy extend CommonPage
• Specific pages in pages/<client>/* extend <Client>CommonPage (NOT CommonPage directly)

Bad (skip the client tier):

// Keywords/katalon/pages/morganstanley/FundingPage.groovy
class FundingPage extends CommonPage { ... }

Good:

class FundingPage extends MorganStanleyCommonPage { ... }

Rule 5: Test case MUST have "Step N:" and "VP:" markers

Every interaction step needs "Step <N>: <action>", every verification needs "VP: <expectation>". These are mandatory for report readability.

Bad:

Page.As(LoginPage).logInBySimulator(creds, data)
Page.As(SomePage).completeRequiredField(data.Fields)

Good:

"Step 1: Log in to the system"
Page.As(LoginPage).logInBySimulator(creds, data)

"VP: Verify the screen opens"
Page.As(SomePage).verifyCurrentPageIsActive(data.PageName)

"Step 2: Complete required fields"
Page.As(SomePage).completeRequiredField(data.Fields)

Decimal steps ("Step 3.1:") are accepted for sub-steps.

Rule 6: Step numbering must be sequential, no gaps, in order

Flag "Step 1" → "Step 3" (missing 2) or "Step 2" → "Step 1" (out of order).

Rule 7: No empty catch / swallowed exceptions

A catch block that does nothing or returns false silently masks failures.

Bad:

try {
    Perform.click(btnSubmit)
} catch (Exception e) {
    // ignored
    return false
}

Good:

try {
    Perform.click(btnSubmit)
} catch (Exception e) {
    KeywordMain.stepFailed("Failed to click Submit: ${e.message}", FailureHandling.STOP_ON_FAILURE)
}

Rule 8: No infinite loop without exit condition

while (true) or for(;;) must have a time-based or counter-based exit.

Bad:

while (true) {
    if (Perform.isVisible(lblSuccess)) break
}

Good:

long startTime = System.currentTimeMillis()
while (System.currentTimeMillis() - startTime < GlobalVariable.LONG_TIMEOUT * 1000) {
    if (Perform.isVisible(lblSuccess)) break
    Thread.sleep(500)
}

Rule 9: No hardcoded timeout literals

Use GlobalVariable.DEFAULT_TIMEOUT / LONG_TIMEOUT / defaultTimeout / longTimeout.

Bad: Perform.click(btnSubmit, 30) Good: Perform.click(btnSubmit, GlobalVariable.LONG_TIMEOUT)

The 12 recurring issues (🟡 Warnings)

For each warning issue, follow the same output pattern (file:line, brief why, diff fix).

🟢 Suggestions (strict mode only)

• Line > 200 chars (codenarc warning threshold)
• Method cyclomatic complexity > 15
• Missing region markers in page class (//*** Page Objects ***, //*** Page Actions ***, //*** Page Assertions ***, //*** Helper Methods ***)
• Variable could be renamed for clarity

What NOT to flag

These patterns are intentional in this codebase — never flag:

• if (true) { ... } blocks — used to group steps for report readability
• Hungarian prefix on UI element variables — ddl*, btn*, lbl*, txt*, lnk*, cbx*, rdo* is the convention
• def for local variables — codenarc allows it (NoDef disabled)
• Missing JavaDoc on classes — codenarc allows (ClassJavadoc, JavadocType disabled)
• Jira-ticket-based test case naming — MSAFRMFA-7848_AUTOKAT-794.groovy is correct
• "Step N: ..." / "VP: ..." as bare Groovy string literals — this IS the convention
• Lines up to 200 chars
• Implicit return statements at end of method
• smartOR(elem) for single-element TestObject conversion
• baseOR(elem) for multi-element / count operations

Output format

Two modes: Markdown (default) and HTML (when user asks to "xuất file", "save", "report", or "html").

Markdown (default)

Respond using exactly this 3-part structure. Severity emojis only — no BLOCKER/WARNING words.

Part (a) — Header (2 lines):

SCOPE: <what was reviewed — see scope guide below>
VERDICT: REQUEST CHANGES | Risk: Medium | 3 issues (1🔴 2🟡)

Part (b) — Summary table:

Part (c) — Detailed findings: one block per issue, ordered by severity then file alphabetically. Each block consists of: severity emoji + file:line — Rule/Issue name on the first line; a 1-line reason on the second line; then a ```diff fence with - bad and + good lines. See the Example section below for a fully rendered instance.

Scope guide (first line — tells reader what was reviewed):

• SCOPE: PR equivalent on feature/AUTOKAT-794 — 5 commits + 1 uncommitted, 4 files, 142 lines (default, feature branch with uncommitted)
• SCOPE: branch commits only — 3 commits on feature/X vs origin/main, 2 files, 87 lines (committed, no uncommitted)
• SCOPE: uncommitted only — 2 files, 34 lines (on main/master, or branch w/o commits yet)
• SCOPE: <file path> (single file specified)
• SCOPE: pasted snippet (~40 lines) (inline code)
• SCOPE: PR #1234 diff (gh pr diff) (PR review)

When reviewing multiple files at once (default mode), order issues by severity first, then by file alphabetically. Don't add per-file headers — the file:line prefix on each issue is enough.

Passed checks line — always present, one line at the very end:

✓ Passed: hard rules · 12 recurring · selector patterns · timeout usage

Strict mode adds 🟢 finding blocks (same format as Part c) between the detailed findings and the passed line.

Optional NOTE: line after the passed line, only if there is non-obvious insight (e.g., "Issue #1 is the only flaky risk; the other two are maintenance"). Skip if the issue list speaks for itself.

HTML (when user requests export)

When the user asks to "xuất file", "save", "report", or "html", write a standalone .html file (e.g., review-AUTOKAT-794.html) that can be opened in any browser. Required structure:

• Header: verdict pill (green = APPROVE, amber = COMMENT-ONLY, red = REQUEST CHANGES) + scope string
• Metric cards: Blockers count · Warnings count · Files reviewed
• Summary table: same columns as Markdown Part (b)
• Finding cards: one card per issue; left border color red (🔴) or amber (🟡); contains rule/issue name, file:line, 1-line why, and a diff block with red background for - lines and green background for + lines
• Inline CSS only — no external dependencies

Default is always Markdown; generate HTML only when explicitly requested.

Verdict guide:

• APPROVE — zero blockers, at most 1 warning
• COMMENT-ONLY — zero blockers + exactly 2 warnings; OR reviewing legacy code at user's request; OR diff context is insufficient to judge
• REQUEST CHANGES — any 🔴 blocker, or 3+ warnings

Risk guide:

• Low — no flaky risk, only style/clarity
• Medium — possible flakiness or maintainability issue
• High — guaranteed flakiness (e.g., infinity loop, swallowed exception in core path)

Example

Input code (Test Cases/Clients/Acme/ACME-100_AUTOKAT-200.groovy, new file):

"PREPARATION"
def creds = DataHelper.JSON.render("DataStorage/Seeds/Acme/ACME-100.json", [:])
def data  = DataHelper.JSON.render("DataStorage/Seeds/Acme/ACME-100.json", [:])

"TEST STEPS"
Page.As(LoginPage).logInBySimulator(creds, data)

Perform.inputText(locator("label:text-is('First Name') + input"), "John")
Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
Perform.selectDropdown(locator("label:text-is('State') + select"), "CA")
Perform.click(btnSubmit)

Page.As(ConfirmationPage).verifyCurrentPageIsActive(data.PageName)

Review output:

SCOPE: pasted snippet (~14 lines) VERDICT: REQUEST CHANGES | Risk: Medium | 3 issues (2🔴 1🟡)

🔴 snippet:8–10 — Rule 1: completeAllRequiredFields Three manual Perform calls replace what the data-driven dispatcher should own; breaks when a field is added or renamed in the form.

- Perform.inputText(locator("label:text-is('First Name') + input"), "John")
- Perform.inputText(locator("label:text-is('Last Name') + input"), "Doe")
- Perform.selectDropdown(locator("label:text-is('State') + select"), "CA")
- Perform.click(btnSubmit)
+ Page.As(ApplicantPage).completeAllRequiredFields(data.ApplicantScreen.Fields)

🔴 snippet:6–13 — Rule 5: Missing Step/VP markers None of the actions or the verification are wrapped in "Step N:" / "VP:" literals; Katalon report will show blank step names.

+ "Step 1: Log in to the system"
  Page.As(LoginPage).logInBySimulator(creds, data)
+ "Step 2: Complete applicant fields"
  Page.As(ApplicantPage).completeAllRequiredFields(data.ApplicantScreen.Fields)
+ "VP: Verify confirmation page is shown"
  Page.As(ConfirmationPage).verifyCurrentPageIsActive(data.PageName)

🟡 snippet:8–10 — Issue 9: escapeSelector check Hardcoded string literals here are safe, but if these selectors reference dynamic data.* values in the real implementation, wrap them with escapeSelector() to guard against quotes or backslashes in data.

- locator("label:text-is('First Name') + input")
+ locator("label:text-is('" + escapeSelector(data.firstName) + "') + input")

✓ Passed: hard rules (3,4,6–9) · 12 recurring (1,2,3,4,5,6,7,8,10,11,12) · timeout usage

Edge cases

• Diff too narrow — if ±3 lines around a change is all that's visible, don't speculate about hidden code. Note: Cannot verify <X> without seeing the full method.
• Test case file is short — if the file only has "PREPARATION" and "TEST STEPS" markers with one step, this is likely a draft. Note Draft state — incomplete and only flag structural blockers.
• Generated/recorder code — if locators are clearly auto-generated (huge absolute XPath, .css-1a2b3c), flag for refactor before merge.
• Framework code (Keywords/katalon/core/*) — apply stricter scrutiny: param types mandatory, Groovydoc mandatory, no def, no unjustified retry.
• Legacy file assessment — if user says "this is legacy, just assess", use COMMENT-ONLY verdict and focus on flaky-risk only.

What this skill does NOT do

• Does not auto-fix code — only suggests; user applies changes.
• Does not run tests — for execution, use katalon-execute-and-fix.
• Does not analyze failure logs — use katalon-failure-analyzer.
• Does not review non-Katalon Groovy (e.g., generic scripts outside this project).

References

• Data location: DataStorage/Seeds/<Client>/<TICKET>.json for input data; DataStorage/Import/<TICKET-ID>/ for files to upload.
• Timeouts: GlobalVariable.DEFAULT_TIMEOUT, GlobalVariable.LONG_TIMEOUT, plus defaultTimeout / longTimeout shortcuts on CommonPage.
• Selector helpers: smartOR(elem) for single TestObject conversion; baseOR(elem) for multi-element/count operations.
• For deeper convention details, see the katalon-convention-linter skill if available.