From 3489c42b048a32d9a5eaca77fcd00e330bf4d00c Mon Sep 17 00:00:00 2001 From: Plamen Ivanov Date: Tue, 23 Jun 2026 17:17:26 +0300 Subject: [PATCH 1/2] feat(ui5): Add QUnit best-practices skill (#XX) Adds the ui5-best-practices-qunit skill covering coding standards for OpenUI5/SAPUI5 unit test files. Follows the same structure as the OPA5 skill: a lean SKILL.md router plus three focused reference files loaded on demand by task type (writing new tests, modernizing existing ones, async patterns). - SKILL.md: trigger description with literal user phrases, core rules table, quick-reference checklist - references/writing-new-tests.md: module structure, AAA pattern, test naming, helpers, file setup (/*global QUnit */, sap.ui.define imports) - references/modernizing-tests.md: var/const/let, bind, assert.async, Core.applyChanges, sinon sandbox, assert.expect, import cleanup, encoding fix, what-not-to-change guard table - references/async-patterns.md: nextUIUpdate vs Core.applyChanges decision table (incl. fake-timer exceptions), waitForEvent, waitForRendering via addEventDelegate, when not to convert assert.async - README.md: adds ui5-best-practices-qunit section - plugin.json / .github/plugin/plugin.json: adds "qunit" keyword - All files ISO 8859-1 compliant (no em dashes or non-ASCII) JIRA: BGSOFUIPIRIN-7067 --- plugins/ui5/.github/plugin/plugin.json | 1 + plugins/ui5/README.md | 13 ++ plugins/ui5/plugin.json | 1 + .../skills/ui5-best-practices-qunit/SKILL.md | 50 ++++++ .../references/async-patterns.md | 151 ++++++++++++++++ .../references/modernizing-tests.md | 167 ++++++++++++++++++ .../references/writing-new-tests.md | 127 +++++++++++++ 7 files changed, 510 insertions(+) create mode 100644 plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md create mode 100644 plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md create mode 100644 plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md create mode 100644 plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md diff --git a/plugins/ui5/.github/plugin/plugin.json b/plugins/ui5/.github/plugin/plugin.json index c28c42f..171e26c 100644 --- a/plugins/ui5/.github/plugin/plugin.json +++ b/plugins/ui5/.github/plugin/plugin.json @@ -14,6 +14,7 @@ "sapui5", "openui5", "opa5", + "qunit", "plugin", "linter", "api-documentation", diff --git a/plugins/ui5/README.md b/plugins/ui5/README.md index e8ea7fb..3dc4fb6 100644 --- a/plugins/ui5/README.md +++ b/plugins/ui5/README.md @@ -42,6 +42,19 @@ Development guidelines for UI Integration Cards (also known as UI5 Integration C - **i18n** - Bind all user-facing strings to the i18n model; never hardcode - **Actions** - Use the `actions` property for links and interactions; never inline `` tags or hand-roll URL handlers +#### ui5-best-practices-qunit + +Coding standards and modernization patterns for QUnit unit test files in UI5 libraries: + +- **Variable declarations** - `const`/`let` over `var`; one declaration per line +- **Async patterns** - `await nextUIUpdate()` over `Core.applyChanges()`; `async/await` over `assert.async()`; `waitForEvent` helper pattern +- **Fake timer safety** - When to keep `Core.applyChanges()` instead of converting (fake timers, shared helpers, load callbacks) +- **assert.expect(N)** - Required in every async test to guard against silent passes +- **Module isolation** - `beforeEach`/`afterEach` lifecycle; `try/finally` teardown in helpers +- **Sinon sandbox** - `sinon.createSandbox()` over deprecated `sinon.sandbox.create()` +- **Test naming** - Descriptive sentences; no "it should"; unique names per module +- **File hygiene** - Remove unused imports; ISO 8859-1 compliance; ESLint 0 errors + #### ui5-best-practices-opa5 Guidelines and debugging workflow for OPA5 integration tests: diff --git a/plugins/ui5/plugin.json b/plugins/ui5/plugin.json index c28c42f..171e26c 100644 --- a/plugins/ui5/plugin.json +++ b/plugins/ui5/plugin.json @@ -14,6 +14,7 @@ "sapui5", "openui5", "opa5", + "qunit", "plugin", "linter", "api-documentation", diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md new file mode 100644 index 0000000..b038074 --- /dev/null +++ b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md @@ -0,0 +1,50 @@ +--- +name: ui5-best-practices-qunit +description: | + Use when the user asks to "write a QUnit test", "fix a failing QUnit test", "add a QUnit module", "modernize QUnit tests", or mentions QUnit-specific constructs such as assert.async, nextUIUpdate, Core.applyChanges, sinon sandbox, or QUnit.module. Covers coding standards for OpenUI5/SAPUI5 unit test files: const/let over var, arrow functions over .bind(this), async/await over assert.async(), assert.expect() in every async test, sinon.createSandbox(), descriptive test names, beforeEach/afterEach module isolation, nextUIUpdate vs Core.applyChanges rules, try/finally teardown in helpers, and ISO 8859-1 compliance. +--- + +# QUnit Test Best Practices for UI5 + +## When to load each reference + +| Trigger | Load | +|---|---| +| Writing a new QUnit test file or module from scratch | [`references/writing-new-tests.md`](references/writing-new-tests.md) | +| Modernizing, refactoring, or reviewing existing test code | [`references/modernizing-tests.md`](references/modernizing-tests.md) | +| Any test touches `nextUIUpdate`, `Core.applyChanges`, `assert.async`, fake timers, or event-based async | [`references/async-patterns.md`](references/async-patterns.md) | + +Load the reference before producing any output. Do not work from memory. + +--- + +## Core rules (always apply) + +| Rule | Detail | +|---|---| +| No `var` | Use `const` or `let`. One declaration per line - no comma chains. | +| No `.bind(this)` | Use arrow functions for callbacks that do not need their own `this`. | +| `assert.expect(N)` in every `async` test | Guards against silent passes when async callbacks never fire. Not required for sync tests. | +| `sinon.createSandbox()` | `sinon.sandbox.create()` is deprecated - never use it. | +| Descriptive test names | Sentence describing behavior. Never start with "it should". Unique within each module. | +| `beforeEach` / `afterEach` in every module | Create all controls in `beforeEach`, destroy them in `afterEach`. No shared mutable state between tests. | +| `try/finally` in helper-created controls | Helpers that create a control must destroy it in `finally` so it is cleaned up even when assertions throw. | +| ISO 8859-1 compliance | No non-ASCII characters in comments, strings, or JSDoc. Use plain ASCII hyphens, not em dashes. | +| ESLint - 0 errors | Warnings for pre-existing patterns (`max-nested-callbacks`, `no-use-before-define`, `valid-jsdoc`) are acceptable. | + +--- + +## Quick-reference checklist + +Use when authoring or reviewing a QUnit test file: + +- [ ] No `var` - use `const` or `let`; one declaration per line (no comma chains) +- [ ] No `.bind(this)` - use arrow functions for callbacks that do not need their own `this` +- [ ] No `assert.async()` in simple cases - use `async function` + `await new Promise(...)` +- [ ] Every `async` test has `assert.expect(N)` +- [ ] No `sinon.sandbox.create()` - use `sinon.createSandbox()` +- [ ] No `"it should..."` test titles - use descriptive sentences +- [ ] Every `QUnit.module` has `beforeEach` / `afterEach` that create and destroy all controls +- [ ] `Core.applyChanges()` kept (not replaced with `nextUIUpdate()`) when `useFakeTimers` is active +- [ ] Helper functions that create controls destroy them in `try/finally` +- [ ] No non-ASCII characters - files must be ISO 8859-1 compliant diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md new file mode 100644 index 0000000..4100045 --- /dev/null +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md @@ -0,0 +1,151 @@ +# Async Patterns in QUnit Tests + +This reference covers every async decision point: rendering, event waiting, fake timers, and when NOT to convert. + +--- + +## 1. Rendering - nextUIUpdate() vs Core.applyChanges() + +**Default:** use `await nextUIUpdate()`. Make the test function `async`. + +```js +// Bad +oControl.setVisible(false); +Core.applyChanges(); +assert.notOk(oControl.getDomRef(), "not rendered"); + +// Good +oControl.setVisible(false); +await nextUIUpdate(); +assert.notOk(oControl.getDomRef(), "not rendered"); +``` + +**Keep `Core.applyChanges()` - do NOT replace - in these cases:** + +| Situation | Why | +|---|---| +| Sinon fake timers active (`sinon.useFakeTimers()`, `sinon.config.useFakeTimers = true`, or sinon's QUnit integration) | `await nextUIUpdate()` queues a microtask that never resolves when the clock is frozen - the test hangs indefinitely. | +| `placeAt()` called in `beforeEach` and assertions run immediately after in the test body | `nextUIUpdate()` only resolves if a render is already pending at the moment of the call. | +| Shared helper functions (`renderObject`, `waitForUIUpdates`) used by many tests | Keep them synchronous so callers can reason about DOM state without async coordination. | +| Inside a `load` event callback that must flush a subsequent `invalidate()` synchronously | `Core.applyChanges()` must be called inside the callback. | + +Note the reason in the commit message when keeping `Core.applyChanges()` so future reviewers do not flag it. + +--- + +## 2. Waiting for control events - waitForEvent() helper + +Wrap one-time event listeners in a Promise helper instead of `assert.async()` + callback boilerplate. + +**Generic helper - define once per file:** + +```js +function waitForEvent(oControl, sEventName) { + return new Promise((resolve) => { + oControl.attachEventOnce(sEventName, resolve); + }); +} +``` + +```js +// Bad +QUnit.test("something", function(assert) { + const done = assert.async(); + oControl.attachEventOnce("someEvent", function() { + assert.ok(oControl.getProperty("x"), "property set"); + done(); + }); +}); + +// Good +QUnit.test("something", async function(assert) { + assert.expect(1); + await waitForEvent(oControl, "someEvent"); + assert.ok(oControl.getProperty("x"), "property set"); +}); +``` + +**ObjectPageLayout example** - `onAfterRenderingDOMReady` fires after internal scroll/height calculations, after the render cycle: + +```js +function waitForDOMReady(oOPL) { + return new Promise((resolve) => { + oOPL.attachEventOnce("onAfterRenderingDOMReady", resolve); + }); +} +``` + +**Waiting for a specific control's next render cycle** - use `addEventDelegate` when the assertion depends on a particular control re-rendering, not just any pending render: + +```js +function waitForRendering(oControl) { + return new Promise((resolve) => { + const oDelegate = { + onAfterRendering: function() { + oControl.removeEventDelegate(oDelegate); + resolve(); + } + }; + oControl.addEventDelegate(oDelegate); + }); +} + +QUnit.test("toolbar updates after model change", async function(assert) { + assert.expect(1); + const oRenderPromise = waitForRendering(this.oToolbar); + this.oModel.setProperty("/title", "Updated"); + await oRenderPromise; + assert.strictEqual(this.oToolbar.getDomRef().textContent, "Updated", "title updated"); +}); +``` + +**FlexibleColumnLayout helpers:** + +```js +function waitForColumnsResize(oFCL) { + return oFCL._oAnimationEndListener.waitForAllColumnsResizeEnd(); +} + +function waitForColumnsResizeOnce(oFCL) { + return new Promise((resolve) => { + oFCL._attachAfterAllColumnsResizedOnce(resolve); + }); +} +``` + +--- + +## 3. assert.async() - when to convert, when to leave + +**Convert** simple one-callback patterns to `async` + `await new Promise(...)`. + +**Do NOT convert** when the callback contains any of: + +| Pattern | Reason to keep assert.async() | +|---|---| +| `setTimeout` with a non-zero delay inside the callback | The delay is intentional - see section 4. | +| Nested `attachEventOnce` calls | Complex event chains are harder to reason about as nested awaits. | +| Multiple `done()` call sites | Cannot be represented as a single Promise resolution. | +| Stubs or spies that call `done()` internally | The done reference is captured inside the stub - removing it breaks the test. | + +--- + +## 4. Intentional setTimeout delays - do not remove + +Some `setTimeout` calls are genuinely necessary: + +- **Post-render DOM calculations:** scroll positions, element heights, resize observer results - these complete asynchronously *after* the post-render event fires. +- **Animation/transition waits:** resize handler processing, CSS animation completions. +- **Fake timer tests:** `sinon.useFakeTimers()` requires `Core.applyChanges()` and explicit `this.clock.tick(n)`. + +When keeping such a `setTimeout`, add a brief inline comment with the actual reason: + +```js +// column resize animation completes asynchronously after afterOpen fires +setTimeout(() => { + assert.strictEqual(oDialog.getDomRef().offsetWidth, iExpected, "width correct"); + done(); +}, 300); +``` + +Do not use generic labels - write the actual cause. diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md new file mode 100644 index 0000000..dbcd075 --- /dev/null +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md @@ -0,0 +1,167 @@ +# Modernizing Existing QUnit Tests + +Follow this reference when refactoring, reviewing, or fixing existing test code. Each section is an independent transformation - apply whichever are relevant. + +--- + +## 1. var -> const / let + +Replace `var` with `const` (never reassigned) or `let` (reassigned). One declaration per line - split comma chains. + +```js +// Bad +var oPage = this.oObjectPage; +var iCount = 0; +const oSection = oPage.getSections()[0], + oSubSection = oSection.getSubSections()[0]; + +// Good +const oPage = this.oObjectPage; +let iCount = 0; +const oSection = oPage.getSections()[0]; +const oSubSection = oSection.getSubSections()[0]; +``` + +When an uninitialized declaration is followed by a single assignment, inline them: + +```js +// Bad +let oItem; +// ... setup ... +oItem = oList.getFirstItem(); + +// Good +const oItem = oList.getFirstItem(); +``` + +--- + +## 2. .bind(this) -> arrow functions + +Replace `.bind(this)` callbacks with arrow functions when the callback does not need its own `this`. + +```js +// Bad +aItems.forEach(function(oItem) { + assert.ok(oItem.getVisible(), "item is visible"); +}.bind(this)); + +// Good +aItems.forEach((oItem) => { + assert.ok(oItem.getVisible(), "item is visible"); +}); +``` + +--- + +## 3. assert.async() -> async/await + +Convert simple one-callback patterns. Leave complex ones unchanged. + +```js +// Bad +QUnit.test("event fires", function(assert) { + assert.expect(1); + const done = assert.async(); + oControl.attachEventOnce("afterOpen", function() { + assert.ok(true, "afterOpen fired"); + done(); + }); + oControl.open(); +}); + +// Good +QUnit.test("event fires", async function(assert) { + assert.expect(1); + await new Promise((resolve) => { + oControl.attachEventOnce("afterOpen", () => { + assert.ok(true, "afterOpen fired"); + resolve(); + }); + }); + oControl.open(); +}); +``` + +**Do NOT convert** when the callback has nested `attachEventOnce` chains, multiple `done()` call sites, or stubs/spies that call `done()` internally. See [`async-patterns.md`](async-patterns.md) for the full rules. + +--- + +## 4. Core.applyChanges() -> await nextUIUpdate() + +Replace `Core.applyChanges()` with `await nextUIUpdate()` and make the function `async`. + +**Do NOT replace** when fake timers are active, the pattern is in a shared helper, or the call is inside a `load` event callback. See [`async-patterns.md`](async-patterns.md) for all exceptions - check them before converting. + +When replacing, add `assert.expect(N)` to the test if it is now `async` and did not have one before. + +--- + +## 5. sinon.sandbox.create() -> sinon.createSandbox() + +```js +// Bad +const oSandbox = sinon.sandbox.create(); + +// Good +const oSandbox = sinon.createSandbox(); +``` + +When the QUnit-sinon integration is already active, `this.stub()`, `this.spy()`, and `this.clock` are available directly on the QUnit context - a manual sandbox is not needed at all. + +--- + +## 6. Add assert.expect(N) to async tests that lack it + +Every `async` test must declare the expected assertion count. Scan the test body to count the assertions and add the call as the first line. + +```js +// Bad - if the await never resolves, 0 assertions pass silently +QUnit.test("change event fires", async function(assert) { + await waitForEvent(oControl, "change"); + assert.ok(oControl.getSelectedItem(), "item selected"); +}); + +// Good +QUnit.test("change event fires", async function(assert) { + assert.expect(1); + await waitForEvent(oControl, "change"); + assert.ok(oControl.getSelectedItem(), "item selected"); +}); +``` + +--- + +## 7. Remove unused imports + +After replacing all `Core.applyChanges()` calls, remove `sap/ui/core/Core` from the `sap.ui.define` array and function parameters - unless `Core` is still used elsewhere (e.g. `Core.byId`, `Core.getConfiguration`). + +--- + +## 8. Fix non-ASCII characters + +Replace em dashes (U+2014) and other non-ASCII characters in comments with plain ASCII hyphens. Files must be ISO 8859-1 compliant. + +```js +// Bad - em dash U+2014 renders as a garbled character +// Exception keep Core.applyChanges() when... + +// Good - plain ASCII hyphen +// Exception - keep Core.applyChanges() when... +``` + +Find violations: +```bash +grep -Pn '[^\x00-\x7E]' src//test/**/*.qunit.js +``` + +--- + +## 9. What NOT to change + +| Pattern | Leave it | +|---|---| +| `Core.applyChanges()` with fake timers active | Converting hangs the test - see [`async-patterns.md`](async-patterns.md) | +| `setTimeout` with a non-zero delay inside an event callback | The delay is intentional - do not remove or replace with `await` | +| `assert.async()` with nested event chains or multiple `done()` sites | Complex control flow cannot be collapsed into a single Promise | +| `Core.applyChanges()` inside `load` event callbacks | Must flush `invalidate()` synchronously | diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md new file mode 100644 index 0000000..d08c8ab --- /dev/null +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md @@ -0,0 +1,127 @@ +# Writing New QUnit Tests + +Follow this reference when creating a new QUnit test file or adding a new `QUnit.module`. + +--- + +## 1. Module structure + +Every module must have `beforeEach` / `afterEach` hooks. No shared mutable state between tests. + +```js +QUnit.module("My Feature", { + beforeEach: async function() { + this.oControl = new MyControl({ /* ... */ }); + this.oControl.placeAt("qunit-fixture"); + await nextUIUpdate(); + }, + afterEach: function() { + this.oControl.destroy(); + this.oControl = null; + } +}); +``` + +**Exception:** If tests attach event handlers before the initial render fires, use synchronous `placeAt` without `await nextUIUpdate()` in `beforeEach` - otherwise the initial event fires before the test can attach its handler. + +--- + +## 2. Test structure - Arrange / Act / Assert + +Add section comments when the test body has distinct phases. Omit for single-assertion tests. + +```js +QUnit.test("title is updated on section change", async function(assert) { + assert.expect(1); + + // Arrange + const oSection = this.oPage.getSections()[1]; + + // Act + this.oPage.setSelectedSection(oSection.getId()); + await nextUIUpdate(); + + // Assert + assert.strictEqual(this.oPage.getTitle().getText(), "Section 2", "title updated"); +}); +``` + +--- + +## 3. Descriptive test names + +Test names must read as sentences describing the verified behavior. + +| Bad | Good | +|---|---| +| `"basic"` | `"renders with default properties"` | +| `"API"` | `"setVisible triggers layout adjustment"` | +| `"it should render correctly"` | `"header is expanded after initial render"` | + +- Never start with "it should" +- Every name within a module must be unique - duplicates cause QUnit to append number suffixes (`"getSelectedItem() 2"`), making failure reports ambiguous + +--- + +## 4. Async tests + +- Make the function `async` and always add `assert.expect(N)` at the top. +- Use `await nextUIUpdate()` for rendering. See [`async-patterns.md`](async-patterns.md) for the full rules including fake-timer exceptions. +- Prefer `await waitForEvent(oControl, "eventName")` over `assert.async()` for event-based patterns. + +--- + +## 5. Helper functions that create controls + +Destroy the control in `finally` so cleanup runs even when assertions throw. + +```js +// Bad - control leaks if assertion throws +const fnTestProperty = function(mOptions) { + QUnit.test("get" + mOptions.property + "()", function(assert) { + assert.strictEqual(mOptions.control.getProperty(mOptions.property), mOptions.expected); + mOptions.control.destroy(); + }); +}; + +// Good +const fnTestProperty = function(mOptions) { + QUnit.test("get" + mOptions.property + "()", function(assert) { + try { + assert.strictEqual(mOptions.control.getProperty(mOptions.property), mOptions.expected); + } finally { + mOptions.control.destroy(); + } + }); +}; +``` + +--- + +## 6. Repeated async patterns - extract a named helper + +For async wait patterns that appear more than once in a file, extract a named helper rather than inlining the boilerplate each time. + +```js +// Canonical pattern +function waitForEvent(oControl, sEventName) { + return new Promise((resolve) => { + oControl.attachEventOnce(sEventName, resolve); + }); +} +``` + +See [`async-patterns.md`](async-patterns.md) for control-specific helpers (ObjectPageLayout, FlexibleColumnLayout). + +--- + +## 7. File setup + +- Add `/*global QUnit */` as the first line so ESLint recognises the QUnit global without requiring an explicit import. +- Declare all dependencies in `sap.ui.define`. Only import `sap/ui/core/Core` if `Core.byId`, `Core.getConfiguration`, or similar is used - do not import it just for `Core.applyChanges()` when `nextUIUpdate()` is used instead. +- File must be ISO 8859-1 compliant - no non-ASCII characters in comments, strings, or JSDoc. Use plain ASCII hyphens, not em dashes (U+2014). + +Verify encoding before committing: +```bash +grep -Pn '[^\x00-\x7E]' src//test/**/*.qunit.js +``` From dd5abe4afa72ac308b495ef9838ff18595349e78 Mon Sep 17 00:00:00 2001 From: Plamen Ivanov Date: Mon, 29 Jun 2026 11:30:18 +0300 Subject: [PATCH 2/2] fix(ui5-qunit): Address PR review comments and correct technical inaccuracies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR review feedback (flovogt, codeworrior): - README: sort skills alphabetically (qunit after opa5) - async-patterns: promote nextUIUpdate(clock) as the mainstream fake-timer approach (not a limited caveat); add nextUIUpdate.runSync() for sync helpers; replace "note in commit message" with per-occurrence inline comment guidance; fix import path to sap/ui/test/utils/nextUIUpdate; note deprecated sap/ui/qunit/utils/nextUIUpdate re-export (since 1.127); add legacy-free UI5 warning; remove misleading placeAt() exception row - modernizing-tests: expand §5 with bridge-vs-sandbox consistency rule and verifyAndRestore guidance; expand §7 to cover non-standard Core param names and sap.ui.getCore() distinction; soften §8 encoding wording from "must be ISO 8859-1" to "avoid non-ASCII"; update grep patterns to reflect real project layouts; add §9 QUnit 1 -> QUnit 2 globals migration - writing-new-tests: add sinon bridge-vs-dependency consistency guidance, CPOUI5FOUNDATION-1204 note, fix grep path patterns - SKILL.md: add QUnit 1 migration trigger, update sinon sandbox/bridge rule, fix fake-timer checklist item, soften encoding wording throughout --- plugins/ui5/README.md | 18 +-- .../skills/ui5-best-practices-qunit/SKILL.md | 13 +- .../references/async-patterns.md | 58 +++++++-- .../references/modernizing-tests.md | 111 +++++++++++++++++- .../references/writing-new-tests.md | 20 +++- 5 files changed, 186 insertions(+), 34 deletions(-) diff --git a/plugins/ui5/README.md b/plugins/ui5/README.md index 3dc4fb6..d10b83a 100644 --- a/plugins/ui5/README.md +++ b/plugins/ui5/README.md @@ -42,6 +42,15 @@ Development guidelines for UI Integration Cards (also known as UI5 Integration C - **i18n** - Bind all user-facing strings to the i18n model; never hardcode - **Actions** - Use the `actions` property for links and interactions; never inline `` tags or hand-roll URL handlers +#### ui5-best-practices-opa5 + +Guidelines and debugging workflow for OPA5 integration tests: + +- **Failure inspection** - Pause-on-failure mode (`sap.ui.test.qunitPause.pauseRule`) keeps the app live at the failure point for browser inspection +- **TestRecorder tooling** - Temporary `sap.ui.testrecorder.ControlTree` integration to inspect the live control tree and generate reliable OPA5 snippets (UI5 ≥ 1.147) +- **Page object organization** - Placement of actions and assertions across views +- **App teardown** - Cleanup patterns in OPA5 journey tests + #### ui5-best-practices-qunit Coding standards and modernization patterns for QUnit unit test files in UI5 libraries: @@ -55,15 +64,6 @@ Coding standards and modernization patterns for QUnit unit test files in UI5 lib - **Test naming** - Descriptive sentences; no "it should"; unique names per module - **File hygiene** - Remove unused imports; ISO 8859-1 compliance; ESLint 0 errors -#### ui5-best-practices-opa5 - -Guidelines and debugging workflow for OPA5 integration tests: - -- **Failure inspection** - Pause-on-failure mode (`sap.ui.test.qunitPause.pauseRule`) keeps the app live at the failure point for browser inspection -- **TestRecorder tooling** - Temporary `sap.ui.testrecorder.ControlTree` integration to inspect the live control tree and generate reliable OPA5 snippets (UI5 ≥ 1.147) -- **Page object organization** - Placement of actions and assertions across views -- **App teardown** - Cleanup patterns in OPA5 journey tests - #### ui5-best-practices-tables Authoritative development guidelines for all UI5 table controls (SAPUI5 1.136+ LTS): diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md index b038074..b7349ad 100644 --- a/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md +++ b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md @@ -1,7 +1,7 @@ --- name: ui5-best-practices-qunit description: | - Use when the user asks to "write a QUnit test", "fix a failing QUnit test", "add a QUnit module", "modernize QUnit tests", or mentions QUnit-specific constructs such as assert.async, nextUIUpdate, Core.applyChanges, sinon sandbox, or QUnit.module. Covers coding standards for OpenUI5/SAPUI5 unit test files: const/let over var, arrow functions over .bind(this), async/await over assert.async(), assert.expect() in every async test, sinon.createSandbox(), descriptive test names, beforeEach/afterEach module isolation, nextUIUpdate vs Core.applyChanges rules, try/finally teardown in helpers, and ISO 8859-1 compliance. + Use when the user asks to "write a QUnit test", "fix a failing QUnit test", "add a QUnit module", "modernize QUnit tests", "migrate from QUnit 1", or mentions QUnit-specific constructs such as assert.async, nextUIUpdate, Core.applyChanges, sinon sandbox, asyncTest, or QUnit.module. Covers coding standards for OpenUI5/SAPUI5 unit test files: const/let over var, arrow functions over .bind(this), async/await over assert.async(), assert.expect() in every async test, sinon.createSandbox(), descriptive test names, beforeEach/afterEach module isolation, nextUIUpdate vs Core.applyChanges rules, try/finally teardown in helpers, QUnit 1 to QUnit 2 globals migration, and non-ASCII character avoidance. --- # QUnit Test Best Practices for UI5 @@ -12,6 +12,7 @@ description: | |---|---| | Writing a new QUnit test file or module from scratch | [`references/writing-new-tests.md`](references/writing-new-tests.md) | | Modernizing, refactoring, or reviewing existing test code | [`references/modernizing-tests.md`](references/modernizing-tests.md) | +| Migrating from QUnit 1 (globals: `test`, `asyncTest`, `ok`, `stop`, `start`) to QUnit 2 | [`references/modernizing-tests.md`](references/modernizing-tests.md) | | Any test touches `nextUIUpdate`, `Core.applyChanges`, `assert.async`, fake timers, or event-based async | [`references/async-patterns.md`](references/async-patterns.md) | Load the reference before producing any output. Do not work from memory. @@ -25,11 +26,11 @@ Load the reference before producing any output. Do not work from memory. | No `var` | Use `const` or `let`. One declaration per line - no comma chains. | | No `.bind(this)` | Use arrow functions for callbacks that do not need their own `this`. | | `assert.expect(N)` in every `async` test | Guards against silent passes when async callbacks never fire. Not required for sync tests. | -| `sinon.createSandbox()` | `sinon.sandbox.create()` is deprecated - never use it. | +| `sinon.createSandbox()` | `sinon.sandbox.create()` is deprecated - never use it. Alternatively use the QUnit-sinon bridge (`this.stub()`, `this.spy()`). Do not mix both approaches in the same module. | | Descriptive test names | Sentence describing behavior. Never start with "it should". Unique within each module. | | `beforeEach` / `afterEach` in every module | Create all controls in `beforeEach`, destroy them in `afterEach`. No shared mutable state between tests. | | `try/finally` in helper-created controls | Helpers that create a control must destroy it in `finally` so it is cleaned up even when assertions throw. | -| ISO 8859-1 compliance | No non-ASCII characters in comments, strings, or JSDoc. Use plain ASCII hyphens, not em dashes. | +| No non-ASCII characters | No non-ASCII characters in comments, strings, or JSDoc. Use plain ASCII hyphens, not em dashes. UTF-8 is required, but non-ASCII in comments has historically caused encoding issues. | | ESLint - 0 errors | Warnings for pre-existing patterns (`max-nested-callbacks`, `no-use-before-define`, `valid-jsdoc`) are acceptable. | --- @@ -42,9 +43,9 @@ Use when authoring or reviewing a QUnit test file: - [ ] No `.bind(this)` - use arrow functions for callbacks that do not need their own `this` - [ ] No `assert.async()` in simple cases - use `async function` + `await new Promise(...)` - [ ] Every `async` test has `assert.expect(N)` -- [ ] No `sinon.sandbox.create()` - use `sinon.createSandbox()` +- [ ] No `sinon.sandbox.create()` - use `sinon.createSandbox()` or use the bridge (`this.stub()`, `this.spy()`) - [ ] No `"it should..."` test titles - use descriptive sentences - [ ] Every `QUnit.module` has `beforeEach` / `afterEach` that create and destroy all controls -- [ ] `Core.applyChanges()` kept (not replaced with `nextUIUpdate()`) when `useFakeTimers` is active +- [ ] With fake timers: prefer `await nextUIUpdate(this.clock)` over `Core.applyChanges()`; only keep `Core.applyChanges()` when `nextUIUpdate(clock)` cannot handle the case - [ ] Helper functions that create controls destroy them in `try/finally` -- [ ] No non-ASCII characters - files must be ISO 8859-1 compliant +- [ ] No non-ASCII characters in comments or strings (UTF-8 required, but non-ASCII causes encoding issues) diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md index 4100045..2d8cd29 100644 --- a/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md @@ -6,7 +6,7 @@ This reference covers every async decision point: rendering, event waiting, fake ## 1. Rendering - nextUIUpdate() vs Core.applyChanges() -**Default:** use `await nextUIUpdate()`. Make the test function `async`. +**Default:** use `await nextUIUpdate()`. Make the test function `async`. Import from `sap/ui/test/utils/nextUIUpdate`. ```js // Bad @@ -20,16 +20,58 @@ await nextUIUpdate(); assert.notOk(oControl.getDomRef(), "not rendered"); ``` +**With fake timers:** pass the sinon clock instance — `nextUIUpdate` will tick it automatically: + +```js +QUnit.module("with fake timers", { + beforeEach: function() { + this.clock = sinon.useFakeTimers(); + }, + afterEach: function() { + this.clock.restore(); + } +}); + +QUnit.test("renders after setVisible", async function(assert) { + assert.expect(1); + oControl.setVisible(true); + await nextUIUpdate(this.clock); // ticks the clock internally + assert.ok(oControl.getDomRef(), "rendered"); +}); +``` + **Keep `Core.applyChanges()` - do NOT replace - in these cases:** -| Situation | Why | -|---|---| -| Sinon fake timers active (`sinon.useFakeTimers()`, `sinon.config.useFakeTimers = true`, or sinon's QUnit integration) | `await nextUIUpdate()` queues a microtask that never resolves when the clock is frozen - the test hangs indefinitely. | -| `placeAt()` called in `beforeEach` and assertions run immediately after in the test body | `nextUIUpdate()` only resolves if a render is already pending at the moment of the call. | -| Shared helper functions (`renderObject`, `waitForUIUpdates`) used by many tests | Keep them synchronous so callers can reason about DOM state without async coordination. | -| Inside a `load` event callback that must flush a subsequent `invalidate()` synchronously | `Core.applyChanges()` must be called inside the callback. | +| Situation | Preferred fix | Why `Core.applyChanges()` stays | +|---|---|---| +| Sinon fake timers active (`sinon.useFakeTimers()`, `sinon.config.useFakeTimers = true`, or sinon's QUnit integration) | Pass the clock: `await nextUIUpdate(this.clock)` | The clock must be ticked to advance past the `setTimeout(0)` used by async rendering. `nextUIUpdate(clock)` does this automatically and is the standard modern approach (widely used across OpenUI5). Only fall back to `Core.applyChanges()` when the render requires more than a single clock tick and `nextUIUpdate(clock)` cannot handle it. | +| Shared helper functions (`renderObject`, `waitForUIUpdates`) used by many tests | Consider `nextUIUpdate.runSync()` (test-only escape hatch — see below) | Callers need synchronous DOM state without async coordination. | +| Inside a `load` event callback that must flush a subsequent `invalidate()` synchronously | — | `Core.applyChanges()` must be called inside the callback. | + +**`nextUIUpdate.runSync()` - synchronous escape hatch for shared helpers (test code only):** + +When a shared helper function must remain synchronous (e.g. `renderObject` called by many tests that cannot all be made async), use `nextUIUpdate.runSync()` instead of `Core.applyChanges()`. It flushes pending renders synchronously and is explicitly intended for test code. It logs a warning each call as a reminder to migrate. + +```js +// In a shared helper - synchronous, test code only +function renderObject(oControl) { + oControl.placeAt("qunit-fixture"); + nextUIUpdate.runSync(); // test-only; logs a warning to encourage migration +} +``` + +Do not use `nextUIUpdate.runSync()` in production code. + +**Important:** `Core.applyChanges()` is not available in legacy-free UI5. When keeping it instead of converting, add an inline comment on each occurrence explaining why it cannot be converted: + +```js +// keep Core.applyChanges() - load event callback, must flush invalidate() synchronously +Core.applyChanges(); +``` + +Do not rely solely on the commit message - 20 different places can have 20 different reasons, and mapping between commit message and code is cumbersome. -Note the reason in the commit message when keeping `Core.applyChanges()` so future reviewers do not flag it. +**Only `sap/ui/test/utils/nextUIUpdate` must be used.** The legacy path `sap/ui/qunit/utils/nextUIUpdate` is a deprecated re-export (since UI5 1.127) that resolves to the same implementation — do not use it in new code. --- diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md index dbcd075..e4e040b 100644 --- a/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md @@ -109,6 +109,13 @@ const oSandbox = sinon.createSandbox(); When the QUnit-sinon integration is already active, `this.stub()`, `this.spy()`, and `this.clock` are available directly on the QUnit context - a manual sandbox is not needed at all. +**Do not mix** the QUnit-sinon bridge (`this.stub()`, `this.spy()`) with an explicitly created sandbox in the same test or module. Use one approach consistently: + +- **Bridge only:** rely on `this.stub()`, `this.spy()`, `this.clock` - the bridge restores everything automatically in `afterEach`. +- **Sandbox only:** create with `sinon.createSandbox()`, call `oSandbox.restore()` in `afterEach` (or `finally`). + +Note: migrating from a manual sandbox to the bridge is straightforward in most cases. The bridge also exposes `this.verifyAndRestore()`. The only scenario where keeping a sandbox is justified is when the sandbox `verify()` method (without restore) is called mid-test - `this.verifyAndRestore()` always restores, so it cannot replace a standalone `verify()` call. + --- ## 6. Add assert.expect(N) to async tests that lack it @@ -134,13 +141,35 @@ QUnit.test("change event fires", async function(assert) { ## 7. Remove unused imports -After replacing all `Core.applyChanges()` calls, remove `sap/ui/core/Core` from the `sap.ui.define` array and function parameters - unless `Core` is still used elsewhere (e.g. `Core.byId`, `Core.getConfiguration`). +After replacing all `Core.applyChanges()` calls, remove `sap/ui/core/Core` from the `sap.ui.define` array and function parameters - unless the import representing `sap/ui/core/Core` is still used elsewhere. + +Check for all uses of the parameter bound to `sap/ui/core/Core` - it may be named `Core`, `oCore`, `CoreInstance`, or anything else. Remove it only when no usage of that parameter remains: + +- `Core.byId(...)`, `Core.getConfiguration()`, `Core.getModel()`, etc. +- `sap.ui.getCore()` calls in the same file do **not** count as a usage of the imported parameter and do not prevent removal. + +```js +// Before +sap.ui.define([ + "sap/ui/core/Core", + "sap/ui/test/utils/nextUIUpdate" +], function(Core, nextUIUpdate) { + // Core.applyChanges() replaced; Core no longer used +}); + +// After +sap.ui.define([ + "sap/ui/test/utils/nextUIUpdate" +], function(nextUIUpdate) { + // ... +}); +``` --- ## 8. Fix non-ASCII characters -Replace em dashes (U+2014) and other non-ASCII characters in comments with plain ASCII hyphens. Files must be ISO 8859-1 compliant. +Replace em dashes (U+2014) and other non-ASCII characters in comments with plain ASCII hyphens. UTF-8 is the required encoding, but non-ASCII characters - especially in comments - have historically caused encoding issues (e.g. garbled output when the `` tag is missing). Keep comments and strings ASCII-only. ```js // Bad - em dash U+2014 renders as a garbled character @@ -150,14 +179,86 @@ Replace em dashes (U+2014) and other non-ASCII characters in comments with plain // Exception - keep Core.applyChanges() when... ``` -Find violations: +Find violations (adapt the path pattern for your project layout): ```bash -grep -Pn '[^\x00-\x7E]' src//test/**/*.qunit.js +# S/4 reuse libraries +grep -Pn '[^\x00-\x7E]' test//**/*.qunit.js + +# Apps +grep -Pn '[^\x00-\x7E]' webapp/test/**/*.qunit.js +# or +grep -Pn '[^\x00-\x7E]' src/main/webapp/test/**/*.qunit.js ``` --- -## 9. What NOT to change +## 9. QUnit 1 -> QUnit 2 globals migration + +QUnit 1 (loaded via `sap/ui/thirdparty/qunit.js` or a test starter with `qunit/version: 1`) exposed `test`, `asyncTest`, `ok`, `equal`, `strictEqual`, and all other assertions as globals. QUnit 2 requires the `QUnit` namespace and passes the `assert` object as a parameter. + +**Global functions → namespaced:** + +```js +// Bad - QUnit 1 globals +test("renders", function() { + ok(oControl.getDomRef(), "rendered"); +}); + +asyncTest("loads data", function() { + expect(1); + oModel.attachEventOnce("requestCompleted", function() { + ok(oModel.getData(), "data loaded"); + start(); + }); +}); + +// Good - QUnit 2 +QUnit.test("renders", function(assert) { + assert.ok(oControl.getDomRef(), "rendered"); +}); + +QUnit.test("loads data", async function(assert) { + assert.expect(1); + await new Promise((resolve) => { + oModel.attachEventOnce("requestCompleted", () => { + assert.ok(oModel.getData(), "data loaded"); + resolve(); + }); + }); +}); +``` + +**Expected assertion count:** + +QUnit 1 accepted the count as the second argument to `test()`. QUnit 2 uses `assert.expect(N)` as the first line of the test body: + +```js +// Bad - QUnit 1 style +test("fires event", 1, function() { ... }); + +// Good - QUnit 2 style +QUnit.test("fires event", function(assert) { + assert.expect(1); + ... +}); +``` + +**`stop()` / `start()` → async/await:** + +Replace `stop()` / `start()` pairs with `async/await` following the pattern in section 3 above. + +| QUnit 1 | QUnit 2 | +|---|---| +| `asyncTest(...)` | `QUnit.test("...", async function(assert) { ... })` | +| `expect(N)` (global) | `assert.expect(N)` | +| `stop()` / `start()` | `await new Promise(...)` | +| `ok(...)` | `assert.ok(...)` | +| `equal(...)` | `assert.equal(...)` | +| `strictEqual(...)` | `assert.strictEqual(...)` | + +--- + +## 10. What NOT to change | Pattern | Leave it | |---|---| diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md b/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md index d08c8ab..a112bfc 100644 --- a/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md +++ b/plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md @@ -117,11 +117,19 @@ See [`async-patterns.md`](async-patterns.md) for control-specific helpers (Objec ## 7. File setup -- Add `/*global QUnit */` as the first line so ESLint recognises the QUnit global without requiring an explicit import. -- Declare all dependencies in `sap.ui.define`. Only import `sap/ui/core/Core` if `Core.byId`, `Core.getConfiguration`, or similar is used - do not import it just for `Core.applyChanges()` when `nextUIUpdate()` is used instead. -- File must be ISO 8859-1 compliant - no non-ASCII characters in comments, strings, or JSDoc. Use plain ASCII hyphens, not em dashes (U+2014). - -Verify encoding before committing: +- Add `/*global QUnit */` as the first line so ESLint recognises the QUnit global without requiring an explicit import. Note: the Foundation team is working on making this unnecessary (CPOUI5FOUNDATION-1204) - once that feature is available, the comment can be removed. +- **Sinon:** use sinon consistently in one of two ways - do not mix them: + - **Via the QUnit-sinon bridge (preferred):** configured in the test starter; sinon is not imported as a dependency. Use `this.stub()`, `this.spy()`, `this.clock` from the QUnit context. + - **Via explicit dependency:** import sinon as a module dependency and do not configure it via the test starter. Do not use the bridge (`this.stub()` etc.) in this case. + - Add `/*global sinon */` only when sinon is used via the bridge (it arrives as a global, not an AMD module). +- Declare all other dependencies in `sap.ui.define`. Only import `sap/ui/core/Core` if `Core.byId`, `Core.getConfiguration`, or similar is used - do not import it just for `Core.applyChanges()` when `nextUIUpdate()` is used instead. +- Avoid non-ASCII characters in comments, strings, or JSDoc - use plain ASCII hyphens, not em dashes (U+2014). UTF-8 is the required encoding, but non-ASCII characters in comments have historically caused encoding issues. + +Verify encoding before committing (adapt path to project layout): ```bash -grep -Pn '[^\x00-\x7E]' src//test/**/*.qunit.js +# S/4 reuse libraries +grep -Pn '[^\x00-\x7E]' test//**/*.qunit.js + +# Apps +grep -Pn '[^\x00-\x7E]' webapp/test/**/*.qunit.js ```