Skip to content

Fix/skills test waitfor#556

Open
parvezmosharafbd wants to merge 20 commits into
fathah:mainfrom
parvezmosharafbd:fix/skills-test-waitfor
Open

Fix/skills test waitfor#556
parvezmosharafbd wants to merge 20 commits into
fathah:mainfrom
parvezmosharafbd:fix/skills-test-waitfor

Conversation

@parvezmosharafbd

Copy link
Copy Markdown

No description provided.

parvezmosharafbd and others added 18 commits June 4, 2026 01:18
This workflow generates SLSA provenance files for projects, satisfying level 3 requirements. It includes steps for building artifacts and generating provenance subjects.
Add a security policy document outlining supported versions and vulnerability reporting.
Bumps the npm_and_yarn group with 1 update in the / directory: [ip-address](https://github.com/beaugunderson/ip-address).


Updates `ip-address` from 10.1.0 to 10.2.0
- [Commits](beaugunderson/ip-address@v10.1.0...v10.2.0)

---
updated-dependencies:
- dependency-name: ip-address
  dependency-version: 10.2.0
  dependency-type: indirect
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <support@github.com>
…pm_and_yarn-32e07c5719

Bump ip-address from 10.1.0 to 10.2.0 in the npm_and_yarn group across 1 directory
Bumps the npm_and_yarn group with 1 update in the / directory: [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite).


Updates `vite` from 7.3.1 to 7.3.2
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v7.3.2/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v7.3.2/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-version: 7.3.2
  dependency-type: direct:development
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <support@github.com>
The test was failing because the Browse tab wasn't guaranteed to exist when
queried immediately after render. This fix wraps the tab query in a waitFor
block to ensure the component has fully rendered and all tabs are available
before proceeding.

Fixes the failing test:
"surfaces the CLI error in the UI when installSkill returns success:false (issue fathah#310 fix)"

The issue was at line 119 where `browseTab` was undefined because the tab
hadn't rendered yet. Now using the same pattern as the first test to ensure
DOM is ready before assertions.
…pm_and_yarn-c4bc6a0a9e

Bump vite from 7.3.1 to 7.3.2 in the npm_and_yarn group across 1 directory
Bumps the npm_and_yarn group with 4 updates in the / directory: [@xmldom/xmldom](https://github.com/xmldom/xmldom), [lodash](https://github.com/lodash/lodash), [postcss](https://github.com/postcss/postcss) and [tmp](https://github.com/raszi/node-tmp).


Updates `@xmldom/xmldom` from 0.8.12 to 0.8.13
- [Release notes](https://github.com/xmldom/xmldom/releases)
- [Changelog](https://github.com/xmldom/xmldom/blob/master/CHANGELOG.md)
- [Commits](xmldom/xmldom@0.8.12...0.8.13)

Updates `lodash` from 4.17.21 to 4.18.1
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.21...4.18.1)

Updates `postcss` from 8.5.8 to 8.5.15
- [Release notes](https://github.com/postcss/postcss/releases)
- [Changelog](https://github.com/postcss/postcss/blob/main/CHANGELOG.md)
- [Commits](postcss/postcss@8.5.8...8.5.15)

Updates `tmp` from 0.2.5 to 0.2.7
- [Changelog](https://github.com/raszi/node-tmp/blob/master/CHANGELOG.md)
- [Commits](raszi/node-tmp@v0.2.5...v0.2.7)

---
updated-dependencies:
- dependency-name: "@xmldom/xmldom"
  dependency-version: 0.8.13
  dependency-type: indirect
  dependency-group: npm_and_yarn
- dependency-name: lodash
  dependency-version: 4.18.1
  dependency-type: indirect
  dependency-group: npm_and_yarn
- dependency-name: postcss
  dependency-version: 8.5.15
  dependency-type: indirect
  dependency-group: npm_and_yarn
- dependency-name: tmp
  dependency-version: 0.2.7
  dependency-type: indirect
  dependency-group: npm_and_yarn
...

Signed-off-by: dependabot[bot] <support@github.com>
Added additional system dependencies for the build process.
…s.test.tsx

The first test was querying DOM elements synchronously before React had
finished rendering the tabs, causing `browseTab` to be undefined and the
test to fail with "AssertionError: expected undefined to be truthy".

This fix wraps the tab selection logic in `waitFor()` to ensure the DOM
is ready before querying, matching the pattern already used later in
the test for the Install button. The second test already had this
pattern correctly implemented.

Fixes issue fathah#310 diagnosis test failure.
@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a flaky test in Skills.test.tsx by wrapping tab-selection DOM queries in waitFor, ensuring the component is fully rendered before assertions run. It also bumps several dependencies (vite to ^7.3.3, package version to 0.5.4) and regenerates the lock file.

  • Test fix: The first test case is correctly updated — querySelectorAll is moved inside waitFor and the browseTab variable is assigned once with a strong HTMLButtonElement type. The second test case has the same fix applied but the original assignment was not removed, leaving a duplicate (dead) reassignment inside the same waitFor block.
  • Dependencies: Routine patch/minor bumps across vite, brace-expansion, lodash, nanoid, postcss, ip-address, and others in the lock file.
  • Other files: Several unrelated files are included in the PR — a machine-specific Python venv, placeholder GitHub workflow files, an unfilled SECURITY.md template, and VS Code debug configs in tests/.

Confidence Score: 5/5

The core test fix is safe and functionally correct; the duplicate assignment in the second test is dead code that does not affect test behavior or results.

The test changes are narrow and low-risk — both test cases still select the correct DOM element and assertions pass. The duplicate browseTab assignment in the second test is inert dead code. Dependency bumps are routine patch/minor updates.

src/renderer/src/screens/Skills/Skills.test.tsx — second test waitFor block has a leftover duplicate assignment that should be cleaned up.

Important Files Changed

Filename Overview
src/renderer/src/screens/Skills/Skills.test.tsx Wraps tab-selection in waitFor for both tests; first test is clean but second test has a duplicate browseTab assignment (dead code left over from an incomplete edit).
package.json Version bumped to 0.5.4 and vite updated from ^7.2.6 to ^7.3.3; routine dependency update.
package-lock.json Lock file regenerated to match package.json bumps; multiple transitive dependency versions updated (brace-expansion, lodash, nanoid, postcss, vite, etc.).
tests/.venv/pyvenv.cfg Machine-specific virtual environment config with hardcoded absolute codespace path; should not be committed (already flagged in a previous review thread).
.github/workflows/webpack.yml Workflow only installs system dependencies and never runs a build; provides no CI value (already flagged in a previous review thread).
.github/workflows/generator-generic-ossf-slsa3-publish.yml SLSA workflow with unmodified template placeholder build steps producing dummy artifacts (already flagged in a previous review thread).
.github/workflows/datadog-synthetics.yml Datadog Synthetics CI workflow; uses secrets references for API keys but the keys issue was already flagged in a prior review thread.
SECURITY.md Unedited GitHub template SECURITY.md with placeholder version numbers (5.1.x/4.0.x) and generic text that does not reflect the actual project's version history or contact information.
tests/.vscode/launch.json VS Code debug launch configuration for the tests directory; project-specific IDE config committed alongside the venv files.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[render Skills component] --> B[waitFor: API calls fired]
    B --> C{waitFor: Browse tab exists?}
    C -- yes --> D[fireEvent.click browseTab]
    C -- retry --> C
    D --> E{waitFor: Install button exists?}
    E -- yes --> F[fireEvent.click installBtn]
    E -- retry --> E
    F --> G1[Test 1: installSkill called with correct args]
    F --> G2[Test 2: skills-error banner shown with CLI message]
Loading

Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/skills-test..." | Re-trigger Greptile

Comment thread .github/workflows/datadog-synthetics.yml Outdated
Comment thread tests/.venv/pyvenv.cfg
Comment thread .github/workflows/generator-generic-ossf-slsa3-publish.yml
Comment thread .github/workflows/webpack.yml
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

@parvezmosharafbd parvezmosharafbd left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

thanks, and always fix upcoming issues.

@pmos69

pmos69 commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator

Thanks for tracking down the flaky Skills test. I agree with the core idea here: the first Browse tab lookup should wait for the async render before clicking it.

I don't think we can merge this branch as-is, though. It is now conflicting with current main, and it includes a lot of unrelated files: GitHub workflow templates, stock SECURITY.md, tests/.venv, tests/.vscode, plus dependency/lockfile changes.

Could you please re-cut this as a tiny PR against current main that only changes src/renderer/src/screens/Skills/Skills.test.tsx?

For that version:

  • keep the waitFor around the first Browse tab lookup
  • remove the duplicate browseTab assignment in the second test
  • drop the temporary "FIX" comment/emoji
  • remove the workflow/security/venv/IDE/dependency files

That should make the actual test fix straightforward to review and merge.

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.

2 participants