From a41039fd7ae5a0a21e2a2923c61372f6348bc189 Mon Sep 17 00:00:00 2001 From: Plamen Ivanov Date: Tue, 23 Jun 2026 17:17:26 +0300 Subject: [PATCH] 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 1afa384..6e852a7 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 720130d..8f86f20 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 1afa384..6e852a7 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 +```