feat(ui5): Add OPA5 guidelines as Skills#64
Conversation
kineticjs
force-pushed
the
opa5
branch
2 times, most recently
from
b3fb084 to
1b917cd
Compare
|
Hi @kineticjs , Please also add
They are pretty much the same. The only diff is that |
| @@ -0,0 +1,38 @@ | |||
| --- | |||
| name: opa5 | |||
| description: ALWAYS load before any OPA5 task — implementing, updating, or verifying a test. Provides best practices for structuring the test and tools for efficient diagnosis of test failures. | |||
There was a problem hiding this comment.
The description is not in third-person form and contains no trigger phrases. The description is the only signal the model uses to decide whether to load the skill, so it must include concrete phrases the user would actually type ("write an OPA5 test", "my OPA5 test fails", "add a page object"). The current text reads as an instruction to the model rather than a description of when the skill applies. Suggested rewrite: This skill should be used when the user asks to "write an OPA5 test", "fix a failing OPA5 test", "add a page object", or mentions OPA5, opaTest, page objects, journeys.
There was a problem hiding this comment.
Thanks, eventually came up with a version that compiles with the skill-reviewer agent
|
|
||
| ### 2. **ALWAYS** enable pause-on-failure mode (all UI5 versions) | ||
| **Purpose:** When enabled, execution pauses on the first test failure and the app remains live in the browser exactly as it was at the point of failure — no teardown, no reload happens automatically. The paused state persists until you explicitly navigate away, so you can inspect the actual UI directly (without reloading) in the browser to compare it against what the test expected. | ||
| **Setup:** Add the following line to your test setup (e.g. before the first `opaTest` of your Journey under test): |
There was a problem hiding this comment.
I'm unsure about why this isn't reccodemnded but -
Second-person voice here — "so you can inspect the actual UI directly" and "until you explicitly navigate away". The guidelines require imperative form throughout the body. Suggest rephrasing to e.g. "so the actual UI can be inspected directly" and "until explicit navigation occurs".
| 1. Enable the test environment setup above and load the test in the browser. | ||
| 2. When the test pauses on failure, inspect the app first — verify the full causal chain with no gaps before changing any code. **ALWAYS** rule out app-side issues before assuming the test is wrong. | ||
| 3. Once each new/updated journey succeeds in isolation, restore all journey imports for final validation. | ||
| 4. Once all journeys pass with imports restored, remove the `sap.ui.testrecorder` library from the app and disable pause-on-failure. |
There was a problem hiding this comment.
ALWAYS appears six or more times across SKILL.md and the reference files. Emphasis only carries weight when it is rare; when every rule is marked critical, none of them register as critical, and the model loses the signal for which rules are genuinely load-bearing. Recommend keeping it only on this line ("ALWAYS rule out app-side issues before assuming the test is wrong"), which counters a real failure mode, and removing it elsewhere where the prose already states the rule plainly. Also note: trailing whitespace at the end of this line (two spaces after "final validation."). Appears to be a markdown line-break, but it is not needed since the next line is a numbered list item rather than a continuation.
There was a problem hiding this comment.
reduced the count to 3 occurrences
| Opa5.extendConfig({ | ||
| autoWait: true, | ||
| viewNamespace: "com.myorg.myapp.view." | ||
| }); |
There was a problem hiding this comment.
The sap.ui.define callback is not closed — the snippet terminates inside the callback body. Anyone copy-pasting will get a syntax error, and the broken bracketing weakens the file as a reliable reference. Either close the block properly, or drop the sap.ui.define wrapper and show only the extendConfig call with a brief note that the bootstrap is omitted.
There was a problem hiding this comment.
closed the callback and added //.... to show the snippet intends to show partial content. Seems more consistent now indeed.
| ✅ Correct Pattern: | ||
| Place the assertion for the `App.view.xml` in page object file `integration/pages/App.js` | ||
|
|
||
| Example 2: |
There was a problem hiding this comment.
Heading punctuation is inconsistent: Example 1 (line 5) has no colon, Example 2: does. Minor, but visible at a glance in a short file.
|
Hey, I ran the official Claude Code |
3 participants
Adding skills for OPA5 integration testing.
JIRA: BGSOFUIPIRIN-6914