feat(ui5): Add QUnit best-practices skill

[plamenivanov91]

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.

Files

• 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), deprecated Core API replacements (Element.getElementById, ControlBehavior.getAnimationMode, Localization.getLanguage)
• 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); Unicode escapes used for semantically meaningful characters.

Review feedback addressed

• sap.ui.getCore() mid-code calls count as a hidden Core dependency and prevent import removal; top-level attachInit/ready calls are out of scope
• beforeEach exception clarified: when an event must be attached before the initial render, move the full setup (attachEvent + placeAt + await nextUIUpdate()) into the test body to avoid QUnit's internal scheduling inserting a render in between
• Deprecated Core API replacements verified against openui5 source

JIRA: BGSOFUIPIRIN-7067

[flovogt]

@codeworrior Could you have a look at this QUnit best practices, please?

[codeworrior]

Overall LGTM, but IMO, there are still a few things to be clarified / added.

[plamenivanov91]

Here is the original qUnit improvement pilot change 6566798 (Gerrit ID).

[flovogt]

@plamenivanov91 please rebase

[codeworrior]

Again, well done! Only a few comments remaining.

[plamenivanov91]

As recommended by @pskelin , I installed skill-creator and tested the PR.

Here is a short AI summary of the test:

With skill-creator I ran a parallel eval (with-skill vs. baseline) across three scenarios using real openui5 test files as reference material.

The skill is effective at overriding Claude's default biases. Without it, Claude consistently falls back to pre-modern UI5 test patterns — Core.applyChanges() instead of nextUIUpdate(), assert.async() + callbacks instead of async/await, missing assert.expect(N), and var declarations. With the skill loaded, all of those are corrected reliably.

Eval 1 — writing a new async test from scratch (6/7 vs 2/7): The clearest win. The baseline used Core.applyChanges(), assert.async(), var, and omitted assert.expect(N). The skill-guided output used async/await, nextUIUpdate, const, and assert.expect(2) — production-ready out of the box.

Eval 2 — modernizing an existing ToggleButton test from openui5: Both outputs did a solid job (const/let, beforeEach/afterEach, destroy). The skill added one meaningful extra: it caught that the file used the deprecated sap/ui/qunit/utils/nextUIUpdate import path and corrected it to sap/ui/test/utils/nextUIUpdate. The baseline missed this.

Eval 3 — QUnit 1 → QUnit 2 migration guide (8/8 vs 5/8): Both covered the core QUnit 2 API changes. The skill additionally covered replacing Core.applyChanges() with nextUIUpdate() including the fake-timer exception (nextUIUpdate(this.clock)) — entirely absent in the baseline.

The gap identified during eval (the sap.ui.getCore() import rule from modernizing-tests.md not being reflected in SKILL.md) has been fixed in this PR as part of the same iteration.

The so called gap is not really relevant, as it's already implemented in the subsidiary files.

Also recommended by @kineticjs , I ran this skill guideline.

Here is the AI summary of it:

1. Description is not third-person
The guide says descriptions must be third person ("Processes Excel files..." not "Use when the user..."). The current description starts with Use when the user asks to... — that's instructional, not third-person. It should read like: "Covers coding standards for OpenUI5/SAPUI5 QUnit test files... Use when writing, modernizing or migrating QUnit tests..."

2. Description mixes "what" and "when" awkwardly
The guide recommends: what it does first, then "Use when..." triggers. Currently it leads with triggers and buries the "what" at the end.

3. No table of contents in the longer reference files
The guide recommends a ToC at the top of any reference file over 100 lines so Claude can navigate it efficiently. modernizing-tests.md and writing-new-tests.md are both well over 100 lines with no ToC.

4. The async-patterns.md reference isn't surfaced clearly enough
The guide recommends the SKILL.md routing table be the primary navigation aid — currently the table does point to it, but only for "any test touches nextUIUpdate". That file is relevant more broadly.

Want me to fix all four of these? The description rewrite is the highest-value one — it directly affects whether the skill triggers reliably.

Basically it's more polish than actual relevant changes. AI recommends leaving the current PR as is, being relevant and useful.

[flovogt]

@codeworrior @d3xter666 looking forward to your review

[codeworrior]

LGTM. My only comment is about a rare edge case.

[flovogt]

@plamenivanov91 you have to ensure proper commit message "- fixed comment" is not valid

[kgogov]

PR #80 review — SKILL.md proposed changes

Diff below (against your current SKILL.md at commit fa6bb16). Every hunk is annotated with the reasoning and cites the source of truth. Copy the whole file into a single review comment on the PR (or split by hunk if you prefer inline suggestions).

---

Full proposed diff

diff --git a/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md
--- a/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md
+++ b/plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md
@@ -1,9 +1,11 @@
 ---
 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", "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.
+  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(), 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 UTF-8 encoding hygiene.
+
+  Keywords: QUnit, QUnit.module, QUnit.test, assert.async, assert.expect, nextUIUpdate, Core.applyChanges, sinon.createSandbox, sinon-qunit-bridge, beforeEach, afterEach, testTimeout, asyncTest, raises, QUnit 1 to QUnit 2, fake timers, useFakeTimers.
 ---

 # QUnit Test Best Practices for UI5

 ## When to load each reference
@@ -23,16 +25,16 @@ Load the reference before producing any output. Do not work from memory.

 | 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. Alternatively use the QUnit-sinon bridge (`this.stub()`, `this.spy()`). Do not mix both approaches in the same module. |
+| `assert.expect(N)` for conditional or load-bearing counts | Add `assert.expect(N)` when assertions run inside conditional branches or nested callbacks that may not execute, or when the assertion count itself is the load-bearing signal (e.g. asserting that a listener was NOT called). For a linear `async/await` chain that ends in an assertion, `QUnit.config.testTimeout` (3s default) already fails hangs and `assert.expect()` is not required. |
+| Prefer `sinon.createSandbox()` | `sinon.sandbox.create()` emits a runtime deprecation warning in Sinon 5+; prefer `sinon.createSandbox()`. The legacy call still functions and is retained by the openui5 sinon-QUnit bridge as a fallback for older Sinon. Alternatively use the QUnit-sinon bridge (`this.stub()`, `this.spy()`, `this.mock()`; `this.clock` only when fake timers are enabled). Do not mix bridge and manual sandbox 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. |
-| 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. |
+| Encoding: UTF-8; ASCII-preferred in comments | Test files are UTF-8 (per `openui5/docs/guidelines.md`). Prefer plain ASCII in comments  -  watch for accidental em dashes when pasting  -  but non-ASCII string literals are permitted and normal for locale/i18n test data (e.g., Arabic Hijri date strings in `DatePicker.qunit.js`). Use `\uXXXX` escapes when the intent is a specific code point. |
 | ESLint  -  0 errors | Warnings for pre-existing patterns (`max-nested-callbacks`, `no-use-before-define`, `valid-jsdoc`) are acceptable. |

 ---

 ## Quick-reference checklist
@@ -40,12 +42,12 @@ Load the reference before producing any output. Do not work from memory.
 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()` or use the bridge (`this.stub()`, `this.spy()`)
+- [ ] `assert.expect(N)` is present when assertions are inside conditional branches or nested callbacks that may not execute, or when the assertion count itself is the load-bearing signal - linear `await` chains rely on `testTimeout`
+- [ ] No `sinon.sandbox.create()` in new code  -  use `sinon.createSandbox()` or the bridge (`this.stub()`, `this.spy()`, `this.mock()`)
 - [ ] No `"it should..."` test titles  -  use descriptive sentences
 - [ ] Every `QUnit.module` has `beforeEach` / `afterEach` that create and destroy all controls
 - [ ] 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 in comments or strings (UTF-8 required, but non-ASCII causes encoding issues)
+- [ ] File is UTF-8 (`file <path>`); comments prefer plain ASCII (spot check with the grep in [`references/writing-new-tests.md`](references/writing-new-tests.md))

---

Rationale, hunk by hunk

Hunk 1 (lines 4–7) — Description block

Two changes:

1. Drop "assert.expect() in every async test" from the summary; rename "non-ASCII character avoidance" → "UTF-8 encoding hygiene". Both mirror the rule-text changes below (Hunks 3 & 5) so the description stays truthful to the actual rules.
2. Add an explicit Keywords: line. Both ui5-best-practices and ui5-best-practices-tables terminate their | description blocks with a Keywords: … line — a documented pattern in Anthropic's Agent Skills authoring guidance that measurably improves skill-router recall. Current block is 699 chars; with the addition it becomes 905 chars, still well under the 1024 hard limit.

Hunk 2, first change (line 30) — assert.expect(N) rule

Softens a blanket rule into the case where it actually helps.

QUnit.config.testTimeout defaults to 3000 ms and fails any test whose awaited promise never resolves — so a linear async/await chain doesn't silently pass. The official assert.expect() doc page even says: "If you're using [assert.async() / assert.verifySteps()] already, consider removing the assert.expect() call."

The narrower rule — assertions inside conditional branches or nested callbacks, or where the count is itself load-bearing (e.g. "listener was NOT called") — is where assert.expect(N) catches false positives that testTimeout can't.

Hunk 2, second change (line 31) — Sinon rule

Two independent fixes:

1. "sinon.sandbox.create() is deprecated – never use it" overstates the deprecation. Sinon 5 through 7 all wrap sandbox.create with deprecated.wrap(...), which prints a runtime console warning but the function still works — verified directly in cdn.jsdelivr.net/npm/sinon@{5.1.1,6.3.5,7.5.0}/lib/sinon.js (legacySandboxAPI.sandbox.create = deprecated.wrap(sandbox.create, "…")). The openui5 bridge itself uses it as a fallback at sap/ui/core/src/sap/ui/qunit/sinon-qunit-bridge.js:86-90. So "prefer, don't ban" is the accurate framing.
2. Bridge injection list is incomplete. Per sinon-qunit-bridge.js:72-78:

   var aProperties = ["mock", "spy", "stub"],
       bUseFakeTimers = sinon.config && sinon.config.useFakeTimers;
   if (bUseFakeTimers) { aProperties.push("clock"); }

   The bridge injects this.stub(), this.spy(), and this.mock(), plus this.clock only when sinon.config.useFakeTimers is truthy.

Hunk 3 (line 35) — Encoding rule

"No non-ASCII characters" contradicts openui5's own docs/guidelines.md.

openui5/docs/guidelines.md:40 states JS files are UTF-8 — ISO-8859-1 applies only to .properties / .hdbtextbundle. Real tests routinely embed non-ASCII UTF-8 for locale coverage — e.g. src/sap.m/test/sap/m/qunit/DatePicker.qunit.js:662-663 uses "2 ربيع الآخر 1438 هـ" as legitimate Hijri date test input. About 20% of sap.m QUnit files contain non-ASCII bytes today; no ESLint rule blocks them.

The genuinely useful rule is comment-scope (catch accidental em-dashes or curly quotes copied from a design doc), not a blanket ASCII-only ban.

Hunk 4 (lines 47–48) — Checklist rows for expect/sinon

Align checklist wording with the softened rules in Hunk 2. Same reasoning; keeps rule text and checklist consistent.

Hunk 5 (line 53) — Checklist encoding row

Align with the softened encoding rule in Hunk 3.

---

Summary of what the diff does

Hunk	Concern	Severity
1	Adds Keywords: for reliable skill triggering (Anthropic guidance); tightens description	Enhancement
2a	assert.expect(N) framing overstated — testTimeout already catches hangs	Correctness
2b	Sinon "never use" too strong; also missing this.mock() in bridge list	Correctness
3	Encoding rule contradicts openui5/docs/guidelines.md and would reject legitimate locale test data	Correctness (blocks real tests)
4	Checklist consistency with 2a/2b	Consistency
5	Checklist consistency with 3	Consistency

Same review pattern is coming for the three reference files if this lands — they contain two more BLOCKER-severity findings (a deadlocked "Good" async example and a fictitious this.verifyAndRestore() API attributed to the bridge) plus several HIGH/MEDIUM items. Happy to iterate on any of the wording above.

[kgogov]

references/writing-new-tests.md proposed changes

Diff below (against your current file at commit fa6bb16). Four hunks, three of them factual corrections. Copy the whole file into a single review comment on the PR.

---

Full proposed diff

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
--- 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
@@ -30,12 +30,10 @@ QUnit.module("My Feature", {

 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());
@@ -57,19 +55,19 @@ Test names must read as sentences describing the verified behavior.
 | `"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
+- Every name within a module must be unique  -  when two tests share a name, QUnit appends a trailing space to the second one to disambiguate the internal test ID (verified in `sap/ui/thirdparty/qunit-2.js:1361-1365`: `if (this.module.tests[i].name === this.testName) { this.testName += " "; }`). A third collision appends two spaces, and so on. Trailing spaces are invisible in most reporters, so failure output ends up 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.
+- Make the function `async` and 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.
+- Add `assert.expect(N)` when assertions run inside conditional branches or nested callbacks that may not execute, or when the assertion count itself is the load-bearing signal (e.g. asserting that a listener was NOT called). For a linear `await` chain, `QUnit.config.testTimeout` (3s default) already fails hangs  -  a blanket `assert.expect(N)` at the top of every async test is not required, and the official QUnit docs suggest considering its removal when `assert.async()` or `assert.verifySteps()` is already in use.

 ---

 ## 5. Helper functions that create controls

@@ -117,17 +115,22 @@ 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. 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 the QUnit-sinon bridge:** an alternative to `sinon.createSandbox()`; the bridge is configured in the test starter and does not require importing sinon as a dependency. The bridge (see `sap/ui/core/src/sap/ui/qunit/sinon-qunit-bridge.js`, lines 72-90) injects `this.spy()`, `this.stub()`, and `this.mock()` from a per-test sandbox that is `verifyAndRestore()`-ed automatically in `afterEach`. `this.clock` is injected **only when** `sinon.config.useFakeTimers` is truthy (the bridge does `if (bUseFakeTimers)`, not a strict `=== true` check) - otherwise it is undefined.
   - **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`. Do not import `sap/ui/core/Core` just for `Core.applyChanges()`  -  use `nextUIUpdate()` instead. Do not use deprecated Core APIs in new tests: replace `Core.byId(id)` with `Element.getElementById(id)` (`sap/ui/core/Element`), `Core.getConfiguration().getAnimationMode()` with `ControlBehavior.getAnimationMode()` (`sap/ui/core/ControlBehavior`), `Core.getConfiguration().getLanguage()` with `Localization.getLanguage()` (`sap/base/i18n/Localization`), and other deprecated configuration getters with their successor module, and other deprecated methods with their documented modern alternatives.
-- 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.
+- Declare all other dependencies in `sap.ui.define`. Do not import `sap/ui/core/Core` just for `Core.applyChanges()`  -  use `nextUIUpdate()` instead. Note that `sap/ui/test/utils/nextUIUpdate` is a **test-only** module (its own JSDoc: *"No module from `sap/ui/test` should be used for productive coding!"*)  -  never import it from application source.
+- Do not use deprecated Core APIs in new tests. Replacements verified against openui5:
+  - `Core.byId(id)` -> `Element.getElementById(id)` (`sap/ui/core/Element`, `@since 1.119`).
+  - `Core.getConfiguration().getAnimationMode()` -> `ControlBehavior.getAnimationMode()` (`sap/ui/core/ControlBehavior`).
+  - `Core.getConfiguration().getLanguage()` -> `Localization.getLanguage()` (`sap/base/i18n/Localization`). For BCP-47 output (e.g. `Accept-Language` headers, HTML `lang` attributes, arguments to `Intl.*`), prefer `Localization.getLanguageTag()` from the same module.
+  - Any other deprecated configuration getter -> its documented successor module.
+- **Encoding:** test files are UTF-8 (per `openui5/docs/guidelines.md`  -  ISO-8859-1 is reserved for `.properties` / `.hdbtextbundle`, not `.js`). Prefer plain ASCII in comments  -  watch for accidental em dashes (U+2014) or curly quotes when pasting. Non-ASCII **string literals** are legitimate and common for locale/i18n test data: e.g., `src/sap.m/test/sap/m/qunit/DatePicker.qunit.js:662-663` uses Arabic Hijri date strings as test input. Use `\uXXXX` escapes when the intent is a specific code point (e.g. `" "` for a non-breaking space).

-Verify encoding before committing (adapt path to project layout):
+Spot-check comment-scope non-ASCII (useful for catching em dashes accidentally pasted from a design doc  -  **not** a compliance gate; matches on string literals are usually fine):
 ```bash
 # S/4 reuse libraries
 grep -Pn '[^\x00-\x7E]' test/<library>/**/*.qunit.js

 # Apps

---

Rationale, hunk by hunk

Hunk 1 (line 33) — Section-2 example: drop the blanket assert.expect(1)

Follows through on the assert.expect(N) framing change in SKILL.md. The example was a linear async/await chain — testTimeout already catches hangs, so the top-of-body assert.expect(1) is noise. Keeping it here would contradict the softened rule.

Hunk 2 (line 60) — Duplicate-name mechanism was wrong

Concrete factual correction against QUnit source. The current text says duplicates get "number suffixes" like "getSelectedItem() 2". Verified against the shipped QUnit at sap/ui/thirdparty/qunit-2.js:1361-1365:

for (i = 0, l = this.module.tests; i < l.length; i++) {
    if (this.module.tests[i].name === this.testName) {
        this.testName += " ";
    }
}

QUnit appends a single space per collision. A second duplicate "foo" becomes "foo "; a third becomes "foo ". Trailing spaces are invisible in most reporters, which is worse than the digit-suffix behavior the current text implies — worth stating accurately.

Hunk 3 (lines 64–68) — Section 4 "Async tests" bullets

Two things:

1. Remove "always add assert.expect(N)" from the top-of-async rule. Same reasoning as SKILL.md Hunk 2a — QUnit.config.testTimeout (3 s default) already fails hangs; QUnit's own assert.expect() doc page says: "consider removing the assert.expect() call" when assert.async() or assert.verifySteps() is in use.
2. Add a new bullet stating the narrower rule. Reserve assert.expect(N) for genuinely conditional/nested-callback assertions and for tests where the count itself is the load-bearing signal (e.g. "listener was NOT called"). This is the rule as re-stated in SKILL.md line 30.

Hunk 4 (lines 118–128) — Section 7 file-setup: sinon, deprecated-Core, encoding

Four independent corrections, grouped because they all live in the same "File setup" bullet list:

1. Sinon bridge label: drop "(preferred)". Contradicts SKILL.md line 31 which recommends sinon.createSandbox() as the preferred approach; the bridge is an alternative. Also expands the injection description with what's actually true per sinon-qunit-bridge.js:72-90: injects this.spy(), this.stub(), and this.mock() (previously omitted); this.clock is injected only when sinon.config.useFakeTimers is truthy (the bridge does if (bUseFakeTimers), not === true).
2. Add the "test-only module" callout for sap/ui/test/*. The nextUIUpdate module's own JSDoc says: "No module from sap/ui/test should be used for productive coding!" — worth surfacing so agents don't blindly import it from application source when they see it in test code.
3. Refactor the deprecated-Core-APIs paragraph into a sub-list, and note Localization.getLanguageTag(). The wall-of-prose form buried the mappings; sub-list is easier to consume. Added the getLanguageTag() note because Localization.getLanguage() returns an ambiguous form (e.g. "en-US" vs "en") — for Accept-Language, HTML lang, and Intl.* args, the module's own JSDoc points to getLanguageTag() (BCP-47) as the preferred choice.
4. Encoding rule: contradicts openui5's own docs/guidelines.md:40. JS files are UTF-8 — ISO-8859-1 applies only to .properties / .hdbtextbundle. Real openui5 tests routinely embed non-ASCII UTF-8 for locale coverage — src/sap.m/test/sap/m/qunit/DatePicker.qunit.js:662-663 uses "2 ربيع الآخر 1438 هـ" as legitimate Hijri date test data (~20% of sap.m QUnit files contain non-ASCII bytes today, none flagged by ESLint). The narrower rule is: prefer ASCII in comments (catch accidental em-dashes from paste), permit non-ASCII in string literals. The grep block is reframed as a comment-scope warning, not a compliance gate.

---

Summary of what the diff does

Hunk	Concern	Severity
1	Consistency with softened assert.expect(N) rule	Consistency
2	QUnit dedup mechanism factually wrong (space, not number)	Correctness
3	assert.expect(N) bullet aligned with SKILL.md rule	Consistency
4	Sinon bridge label + this.mock() omission + Localization.getLanguageTag() + encoding rule vs openui5/docs/guidelines.md	Correctness (blocks real tests via encoding rule)

[kgogov]

@plamenivanov91

I have two more review batches ready for the reference files.
To keep the PR readable I'll send them by DM.

[codeworrior]

Hunk #3 in the review of SKILL.MD (about the encoding rule) is IMO not correct. It's definitely not a blocker in JavaScript code. Exactly one of the locale tests was a good example from the past to follow the suggestedrule:
UI5/openui5@2dac358

Meanwhile, we've enhanced the server component and its sends UTF-8 or ISO8859-1 encoding headers with each resource. But that might not be the case in all servers / on all platforms.

Hunk #2 in the second review (writing-new-tests.md) is worth to be fixed (for correctness), but gives even stronger reasoning for the point of the skill. Differences only in blanks won't be noticeable for a human

[codeworrior]

I missed that before. When sinon is used via the bridge, you normally don't have a need to use the global sinon. Almost everything can be done via the bridge (this).

The global sinon is often used in a 3rd variant (which might be the most common in UI5 tests): sinon is configured via test starter, but not used via the bridge.

[kgogov]

Hello everyone,

The code review comments I submitted are based on an AI analysis using Opus 4.7 and dynamic workflows. The analysis was executed multiple times to increase confidence and reduce the likelihood of hallucinations. Afterward, I manually reviewed all suggested findings and changes.

Since this PR is quite large, it is entirely possible that some of my observations contain inaccuracies or incorrect assumptions. Please treat this information as an additional perspective and supplemental analysis rather than a definitive conclusion.

I believe there is still value in sharing it, as AI can sometimes identify issues or potential improvements that might be overlooked during a manual review.

Important note: this analysis should not be considered a blocker for merging the change in any way. Its purpose is purely advisory and intended only to provide additional information for consideration.

I would also like to note that, in hindsight, it was probably not the best idea to clutter the PR itself with such large analyses. Going forward, I will consider a more appropriate way of sharing this kind of information so that it remains useful without making the review process harder to follow. ✌️