Building an enterprise test automation framework is one thing; keeping it reliable, maintainable, and fast in CI over time is another.
To prevent test rot, flakiness, and architectural drift, code reviews need to evaluate more than just standard syntax. They must enforce design boundaries, state isolation, and robust synchronization strategies.
Here is the complete Code Review Checklist we use for our Playwright + JavaScript + Cucumber BDD suites. Feel free to adopt or adapt it for your team's workflow!
Playwright JavaScript Framework — Code Review Checklist
Use this checklist during pull request reviews to keep tests reliable, maintainable, secure, and CI-friendly.
Scope
New Playwright test scenarios
Updated Cucumber feature files and step definitions
Changes to page objects, hooks, utilities, config, and test data
Reporting, retry, CI, and environment-related automation changes
Review Checklist
1. Test Intent and Coverage
Does the change clearly cover a real business workflow or acceptance criterion?
Is the scenario scoped to a single behavior instead of combining multiple unrelated validations?
Are positive, negative, and edge-case paths considered where appropriate?
Are feature names, scenario names, and tags meaningful and consistent?
Is there any duplication with an existing scenario that should be reused or refactored?
2. Feature File Quality (Cucumber)
Are Given, When, and Then steps written from the user's perspective?
Are scenarios concise, readable, and free of implementation details?
Are tags applied correctly for execution targeting (e.g., @smoke, @admin, @performer, @approver)?
Is Background used only for simple shared preconditions?
Are hardcoded credentials, URLs, IDs, or sensitive values avoided in feature files?
3. Step Definition Quality
Are step definitions thin and delegated to page objects or utilities?
Are duplicate or overly similar step definitions avoided?
Is shared scenario state stored on the Cucumber World object (this) instead of globals?
Are step definitions readable and reusable across scenarios?
Do step definitions avoid embedding large selectors, raw waits, or excessive business logic?
4. Page Object Design
Are UI interactions encapsulated in the correct page object under page-objects/?
Are selectors centralized in the page object instead of duplicated across steps?
Are page object methods named by user intent (e.g., createGroup, approveTask, searchQuery)?
Are page objects focused on actions and state access rather than test assertions?
If a page object became too large, should it be split into smaller components or helper classes?
5. Selector Strategy
Are selectors stable and intention-revealing?
Are data-qa, data-test, data-testid, or semantic Playwright locators preferred?
Are fragile selectors such as positional XPath, generated classes, or deep CSS chains avoided?
If a stable selector was not available, is the fallback rationale documented?
Are selector updates aligned with SELECTORS.md?
6. Assertions and Validation
Are assertions using Playwright's expect with web-first behavior?
Do assertions verify outcomes that matter to the user or workflow?
Are assertions placed in the appropriate layer rather than hidden inside unrelated helpers?
Are weak checks avoided, such as validating only that a page loaded without confirming expected data or state?
Where useful, do assertion messages or attachments help future debugging?
7. Waiting and Synchronization
Does the code rely on Playwright auto-waiting wherever possible?
Are explicit waits tied to real signals like element state, navigation, or API responses?
Are arbitrary sleeps such as waitForTimeout() avoided?
If network-dependent behavior exists, does the code wait on the relevant response or UI state change?
Could any race condition appear under CI speed or parallel execution?
8. Test Isolation and State Management
Can the scenario run independently and in any order?
Is test setup and teardown handled through hooks where appropriate?
Are browser context, page, storage state, and created records isolated per scenario?
Is cleanup handled for created users, groups, files, or seeded data when needed?
Does the change introduce hidden dependencies on previously executed tests?
9. Test Data Handling
Is test data externalized to test-data/, factories, or utilities where appropriate?
Are unique identifiers used to avoid collisions in shared environments?
Are hardcoded dates, names, or IDs avoided unless they are intentionally fixed test fixtures?
Is seeded data documented and compatible with the current environment?
Are data generation and cleanup steps deterministic and maintainable?
10. Configuration and Environment Safety
Are environment-specific values sourced from config.js or environment variables?
Are .env values kept out of committed code and documentation examples unless intentionally sanitized?
Does the change behave safely across local, QA, and CI environments?
Are timeouts, retries, browser settings, and base URLs configurable rather than hardcoded?
Is the documentation updated if the new change adds or alters configuration requirements?
11. Authentication and Secrets
Are credentials sourced only from environment variables or approved secret stores?
Are login flows reusable and abstracted where appropriate?
Are session files, tokens, or auth-state artifacts excluded from source control if sensitive?
Are sensitive values omitted from logs, screenshots, attachments, and console output?
Is the implementation aligned with SECRETS.md and ENV_SETUP.md?
12. Flakiness, Retries, and Reliability
Does the change reduce or increase flakiness risk?
Are retries treated as a safety net rather than the primary fix?
If a flaky area is touched, was the root cause addressed instead of masking the problem?
Is the scenario safe to run under current workers and retry settings?
Is the change consistent with FLAKINESS.md?
13. Logging, Debugging, and Failure Diagnostics
Will a future failure be easy to diagnose from logs, screenshots, traces, and reports?
Are screenshots, traces, or attachments captured at the right level when failures occur?
Does the code use structured logging utilities instead of noisy console.log statements?
Are debug artifacts meaningful and free of sensitive data?
If the reviewer had to debug this in CI, would the report provide enough context?
14. Code Quality and Maintainability
Is the code easy to read, with clear function and variable names?
Are functions small and focused on one responsibility?
Is duplication avoided through helpers, shared steps, or utility methods?
Is the module style consistent with the codebase conventions?
Are unused imports, dead code, commented-out blocks, and temporary debugging code removed?
15. Hooks, Utilities, and Shared Framework Code
If setup/hooks.js or shared utilities changed, have downstream impacts been considered?
Are hooks minimal, predictable, and safe for all tagged scenarios they affect?
Do shared utilities remain generic enough for reuse?
Could the change unintentionally affect unrelated modules like Admin, Performer, Approver?
If framework behavior changed, should related docs or runbooks be updated?
16. Parallelism and CI Compatibility
Will the change behave correctly when tests run in parallel or rerun on failure?
Are file paths, downloads, screenshots, and artifacts unique per test where needed?
Does the change assume a local-only environment or headed execution?
Is the scenario suitable for CI headless mode?
Are run tags and suite grouping still appropriate for smoke vs broader regression?
17. Reporting and Observability
Are Allure and other reporting outputs still generated correctly?
Are important artifacts attached for failures without creating excessive noise?
Does the change preserve report readability and trend usefulness?
If new metadata, labels, or attachments were added, are they helpful and consistent?
Are report-related updates aligned with current scripts and CI expectations?
18. Security, Compliance, and Governance
Does the change avoid exposing confidential business data in code, fixtures, or reports?
If AI-assisted code was introduced, does it receive human review per AI_GOVERNANCE.md?
Are compliance-sensitive workflows documented clearly enough for audit or review if needed?
Does the change follow team expectations captured in DoD.md?
Reviewer Quick Reference
Before hitting Approve, ask yourself:
- Would this still be understandable to a new automation engineer in three months?
- Would this remain reliable in CI under retry and parallel execution?
- Would a failure provide enough evidence to debug quickly?
- Did this change add unnecessary complexity to the framework?
- Is the root cause solved, or is the PR only hiding symptoms?
How do you handle PR reviews for your automation suites? Let me know in the comments!
