feat(ui5): Add QUnit best-practices skill

plugins/ui5/.github/plugin/plugin.json

@@ -14,6 +14,7 @@
         "sapui5",
         "openui5",
         "opa5",
+        "qunit",
         "plugin",
         "linter",
         "api-documentation",

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 `<a>` 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:

plugins/ui5/plugin.json

@@ -14,6 +14,7 @@
         "sapui5",
         "openui5",
         "opa5",
+        "qunit",
         "plugin",
         "linter",
         "api-documentation",

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

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.