Skip to content

feat(ui5): Add QUnit best-practices skill#80

Open
plamenivanov91 wants to merge 4 commits into
UI5:mainfrom
plamenivanov91:qunit-skill
Open

feat(ui5): Add QUnit best-practices skill#80
plamenivanov91 wants to merge 4 commits into
UI5:mainfrom
plamenivanov91:qunit-skill

Conversation

@plamenivanov91

Copy link
Copy Markdown

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

@d3xter666 d3xter666 requested review from a team and flovogt June 23, 2026 14:33
Comment thread plugins/ui5/README.md

@flovogt flovogt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@codeworrior codeworrior left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/async-patterns.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/SKILL.md
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
…curacies

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
@plamenivanov91

plamenivanov91 commented Jul 1, 2026

Copy link
Copy Markdown
Author

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

@flovogt

flovogt commented Jul 6, 2026

Copy link
Copy Markdown
Member

@plamenivanov91 please rebase

Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/modernizing-tests.md Outdated
Comment thread plugins/ui5/skills/ui5-best-practices-qunit/references/writing-new-tests.md Outdated
@codeworrior

Copy link
Copy Markdown
Member

Again, well done! Only a few comments remaining.

- Correct sap.ui.getCore() guidance: mid-code calls are a hidden
  dependency on sap/ui/core/Core and prevent import removal; carve
  out bootstrap exception (attachInit/ready)
- Expand non-ASCII guidance: allow Unicode escapes (e.g. \u00a0) for
  semantically meaningful characters instead of blanket ASCII-only
- New tests: prohibit deprecated Core APIs; name concrete replacements
  (Element.getElementById, ControlBehavior.getAnimationMode,
  Localization.getLanguage)
@plamenivanov91

plamenivanov91 commented Jul 13, 2026

Copy link
Copy Markdown
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants