feat(ui5): Add QUnit best-practices skill#80
Open
plamenivanov91 wants to merge 2 commits into
Open
Conversation
flovogt
reviewed
Jun 23, 2026
flovogt
reviewed
Jun 23, 2026
Member
There was a problem hiding this comment.
@codeworrior Could you have a look at this QUnit best practices, please?
codeworrior
reviewed
Jun 24, 2026
codeworrior
left a comment
codeworrior
left a comment
Member
There was a problem hiding this comment.
Overall LGTM, but IMO, there are still a few things to be clarified / added.
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
plamenivanov91
force-pushed
the
qunit-skill
branch
from
d894c0d to
96cbd54
Compare
…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
force-pushed
the
qunit-skill
branch
from
96cbd54 to
dd5abe4
Compare
Author
|
Here is the original qUnit improvement pilot change 6566798 (Gerrit ID). |
Member
|
@plamenivanov91 please rebase |
codeworrior
reviewed
Jul 9, 2026
| 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. |
Member
There was a problem hiding this comment.
I don't agree to this. When sap.ui.getCore() is used mid-code, a dependency to sap/ui/core/Core should be maintained (or better, sap.ui.getCore() should be replaced with the dependency. Otherwise, sap.ui.getCore() is a hidden dependency to sap/ui/core/Core.
For bootstrap code (top level sap.ui.getCore().attachInit(...) or .ready(...)), replacement is more difficult and out of scope for this skill.
codeworrior
reviewed
Jul 9, 2026
| ## 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 `<meta charset>` tag is missing). Keep comments and strings ASCII-only. |
Member
There was a problem hiding this comment.
if non-ASCII characters are needed, use unicode escapes (e.g. \u00a0)
codeworrior
reviewed
Jul 9, 2026
| - **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. |
Member
There was a problem hiding this comment.
AS this file is about new tests: new tests shouldn't use the deprecated Core functions. byId, getConfiguration or similar, they should use the modern alternatives.
Member
|
Again, well done! Only a few comments remaining. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).
JIRA: BGSOFUIPIRIN-7067