Skip to content

feat(e2e): add plugin sanity check test (RHIDP-13508)#4967

Open
gustavolira wants to merge 13 commits into
redhat-developer:mainfrom
gustavolira:main
Open

feat(e2e): add plugin sanity check test (RHIDP-13508)#4967
gustavolira wants to merge 13 commits into
redhat-developer:mainfrom
gustavolira:main

Conversation

@gustavolira

@gustavolira gustavolira commented Jun 17, 2026

Copy link
Copy Markdown
Member

Summary

Implements enhanced sanity checks for all enabled plugins in the RHDH deployment as part of RHIDP-13508.

Changes

  • New test file: e2e-tests/playwright/e2e/plugin-sanity-check.spec.ts

    • Validates all enabled packages from default.packages.yaml
    • Checks package name format for all enabled plugins
    • Validates disabled packages list is parseable
  • Integration: Added to SHOWCASE_SANITY_PLUGINS project in playwright.config.ts

  • Mock file: Created default.packages.yaml in repo root for local testing

    • Real file is injected during CI deployment
    • Mock allows local test execution

Test Behavior

The test runs in two parts:

  1. Enabled packages validation: Reads all enabled packages and validates their format
  2. Disabled packages validation: Ensures the disabled list is parseable

Currently validates package structure. Future enhancement can add actual plugin loading using @red-hat-developer-hub/cli-module-install-dynamic-plugins.

Testing

Local

yarn playwright test plugin-sanity-check --project showcase-sanity-plugins

CI

Test runs automatically in nightly showcase-sanity-plugins job where RHDH is deployed.

Jira

Checklist

  • Test created with proper component annotation
  • Integrated into existing Playwright project
  • ESM compatibility fixed (__dirname polyfill)
  • Mock file created for local testing
  • Test passes locally with mock data
  • Follows playwright-locators best practices
  • Follows ci-e2e-testing conventions

@openshift-ci openshift-ci Bot requested review from rm3l and subhashkhileri June 17, 2026 20:08
@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

feat(e2e): add Playwright plugin sanity-check test
🧪 Tests ✨ Enhancement ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

Description

• Add Playwright sanity test that parses default.packages.yaml and validates plugin entries.
• Wire the new spec into the SHOWCASE_SANITY_PLUGINS Playwright project for nightly runs.
• Provide a mock default.packages.yaml in repo root to enable local execution.
Diagram

graph TD
  Config["playwright.config.ts"] --> Project["SHOWCASE_SANITY_PLUGINS"] --> Runner["Playwright runner"] --> Test["plugin-sanity-check.spec.ts"] --> Yaml[("default.packages.yaml")] --> Validate["Validate package entries"]
  CI["Nightly CI deployment"] --> Yaml
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Resolve/install validation (require.resolve / dynamic import)
  • ➕ Catches missing deps and broken package exports, not just naming issues
  • ➕ Aligns better with the header comment about “can be loaded”
  • ➖ May be flaky if the test environment doesn’t include all plugin packages
  • ➖ Could increase runtime and require dependency management in CI images
2. Use dynamic plugin installer CLI in the test
  • ➕ Validates the real dynamic-plugin acquisition path end-to-end
  • ➕ Closer to how RHDH consumes plugins in deployment
  • ➖ Adds infra complexity (network access, caching, time)
  • ➖ Harder to keep deterministic in CI without pinning and mirrors
3. Schema/structure-only validation + stronger rules
  • ➕ Keeps the test lightweight and deterministic
  • ➕ Can still catch common config errors (missing fields, duplicates, invalid npm scope/name regex)
  • ➖ Won’t detect runtime load failures or missing packages

Recommendation: The PR’s lightweight approach is reasonable for a nightly “sanity” gate, but the spec currently only checks package-name scoping (starts with @) and disabled-list structure. Consider either (a) tightening the structure validation (required fields, non-empty lists, duplicate detection, npm package-name regex) and adjusting the test header comment to match behavior, or (b) adding an optional require.resolve/import-based check when the environment guarantees plugin deps are present. Full CLI-based installation is higher fidelity but likely too heavy unless the team is ready to invest in caching and determinism.

Files changed (3) +149 / -0

Tests (1) +137 / -0
plugin-sanity-check.spec.tsAdd Playwright spec to validate enabled/disabled plugin entries +137/-0

Add Playwright spec to validate enabled/disabled plugin entries

• Introduces a new Playwright spec that reads 'default.packages.yaml', validates enabled plugin package name format (scoped), and asserts the disabled package list is parseable and well-formed. Adds a 'component=plugins' annotation and emits a simple console summary.

e2e-tests/playwright/e2e/plugin-sanity-check.spec.ts

Other (2) +12 / -0
default.packages.yamlAdd mock default plugin package list for local runs +11/-0

Add mock default plugin package list for local runs

• Adds a repo-root 'default.packages.yaml' with enabled/disabled plugin entries to support local execution of the new Playwright sanity test. Notes that the real file is injected during CI deployment.

default.packages.yaml

playwright.config.tsInclude plugin sanity spec in SHOWCASE_SANITY_PLUGINS project +1/-0

Include plugin sanity spec in SHOWCASE_SANITY_PLUGINS project

• Extends the 'SHOWCASE_SANITY_PLUGINS' project 'testMatch' list to include 'plugin-sanity-check.spec.ts', ensuring it runs as part of the sanity-plugins CI job.

e2e-tests/playwright.config.ts

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.25%. Comparing base (0125ff6) to head (6628116).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4967      +/-   ##
==========================================
- Coverage   55.82%   55.25%   -0.58%     
==========================================
  Files         121      109      -12     
  Lines        2350     2132     -218     
  Branches      539      537       -2     
==========================================
- Hits         1312     1178     -134     
+ Misses       1033      954      -79     
+ Partials        5        0       -5     
Flag Coverage Δ
rhdh 55.25% <ø> (-0.58%) ⬇️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0125ff6...6628116. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Jun 17, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

Grey Divider


Remediation recommended

1. No package resolution performed ✓ Resolved 🐞 Bug ≡ Correctness
Description
The test "All enabled packages can be resolved" never resolves/imports any packages; it only checks
that the string starts with "@" and then records success. This can let CI pass even when a listed
plugin package is missing/unresolvable, making the new sanity check misleading.
Code

e2e-tests/playwright/e2e/plugin-sanity-check.spec.ts[R44-83]

+  test("All enabled packages can be resolved", async () => {
+    // Read default.packages.yaml from rhdh repo root
+    const defaultPackagesPath = join(__dirname, "../../../default.packages.yaml");
+    const yamlContent = readFileSync(defaultPackagesPath, "utf8");
+    const config = yaml.parse(yamlContent) as DefaultPackagesConfig;
+
+    const enabledPackages = config.packages.enabled;
+    console.log(`\n📦 Testing ${enabledPackages.length} enabled packages...\n`);
+
+    const results: {
+      package: string;
+      status: "success" | "failed";
+      error?: string;
+    }[] = [];
+
+    for (const pkg of enabledPackages) {
+      const packageName = pkg.package;
+
+      try {
+        // Attempt to resolve the package
+        // Note: We can't actually import dynamic plugins here as they require
+        // a Backstage runtime, but we can at least verify the package name format
+        // and that it's listed in package.json dependencies
+
+        // Validate package name format
+        if (!packageName.startsWith("@")) {
+          throw new Error("Package name must be scoped (start with @)");
+        }
+
+        // For now, just verify the package is properly formatted
+        // Future enhancement: Use @red-hat-developer-hub/cli-module-install-dynamic-plugins
+        // to actually download and verify the plugins load
+
+        results.push({
+          package: packageName,
+          status: "success",
+        });
+
+        console.log(`✅ ${packageName}`);
+      } catch (error) {
Relevance

⭐⭐ Medium

Mixed history: team accepts improving E2E assertions/coverage (#3147,#4526) but rejected similar
“add runtime validation” checks (#4519).

PR-#3147
PR-#4526
PR-#4519

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The test’s core loop never calls any module-resolution API; it only validates a prefix and then
pushes a success result, so it cannot detect missing/unresolvable packages. Additionally, the E2E
test package itself does not declare the example plugin packages as dependencies, reinforcing that
the current implementation cannot be performing real resolution.

e2e-tests/playwright/e2e/plugin-sanity-check.spec.ts[44-83]
e2e-tests/package.json[57-71]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test name/comments say it verifies packages can be resolved/loaded, but the implementation only checks a naming convention (`startsWith('@')`) and always marks entries as `success` if that passes. This can silently miss real breakages (missing package, typo, unpublished artifact), because no `resolve`/`import` is ever attempted.

### Issue Context
- This is a Playwright E2E "sanity" test intended for CI signal quality. If the test claims resolvability but doesn’t actually do it, it provides misleading green runs.

### Fix Focus Areas
- e2e-tests/playwright/e2e/plugin-sanity-check.spec.ts[44-83]

### Suggested fix
Choose one (A is lowest-risk for now):

**A) Rename + adjust messaging to match reality**
- Rename the test to something like `All enabled packages have valid package identifiers`.
- Update the header comment and inline comments to remove "resolve/import" language.
- Update summary logs to reflect "validated format" rather than "loaded/resolved".

**B) Actually resolve packages (only if intended + feasible)**
- Implement a real resolution check (e.g., `createRequire(import.meta.url).resolve(packageName)` or Node’s `import.meta.resolve` where available) and fail on thrown errors.
- If doing this, ensure the packages being checked are expected to be present in the environment (dependencies or fetched artifacts), otherwise the test will become permanently red.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

gustavolira added a commit to gustavolira/rhdh that referenced this pull request Jun 18, 2026
Implements review findings from PR redhat-developer#4967 code review:

**Correctness fixes:**
- Fix test.beforeAll signature to use test.info() (follows smoke-test.spec.ts pattern)
- Remove unused manifestPath variable in plugin-dynamic-loading.spec.ts

**Code organization:**
- Extract DEFAULT_PACKAGES_PATH constant to avoid duplication
- Move CONFIG_OVERRIDES to module scope for better reusability
- Remove unused catalog-index-parser.ts (can be recreated when needed)

**Type safety:**
- Import and use BackendFeature type for require() calls instead of any
- Improves type safety when loading plugin modules

**Developer experience:**
- Improve console.warn message to explain impact of missing _nodeModulePaths

All changes are low-risk refactorings that improve code quality without
changing behavior. Type checking passes with no errors.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

@gustavolira

Copy link
Copy Markdown
Member Author

/test ?

@gustavolira

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

1 similar comment
@gustavolira

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@gustavolira

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@github-actions

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

gustavolira and others added 6 commits June 19, 2026 16:42
Implements enhanced sanity checks for all enabled plugins in the RHDH deployment.

Changes:
- Added plugin-sanity-check.spec.ts that validates all enabled packages from default.packages.yaml
- Integrated into SHOWCASE_SANITY_PLUGINS project in playwright.config.ts
- Created mock default.packages.yaml for local testing (real file injected in CI)
- Validates package name format for all enabled plugins
- Validates disabled packages list is parseable

The test provides basic sanity checking without requiring complex infrastructure.
Future enhancement can add actual plugin loading using @red-hat-developer-hub/cli-module-install-dynamic-plugins.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Apply modern TypeScript style:
- Convert interface to type (PackageEntry, DefaultPackagesConfig)
- Follows project TypeScript conventions

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements full plugin loadability validation using catalog index and startTestBackend.

## New Files

**Test:**
- `plugin-dynamic-loading.spec.ts` - Main test that loads all plugins from catalog index and validates backend startup

**Utilities:**
- `plugin-loader.ts` - Plugin loading, validation, and config building utilities
- `module-resolution-patch.ts` - Module resolution patch for OCI plugins to find peer deps
- `catalog-index-parser.ts` - Catalog index fetching and parsing (for future use)

## Features

1. **Downloads plugins** from CATALOG_INDEX_IMAGE using @red-hat-developer-hub/cli-module-install-dynamic-plugins
2. **Loads backend plugins** and validates they have proper default exports
3. **Starts test backend** with @backstage/backend-test-utils and all loaded plugins
4. **Validates frontend plugins** have required bundle artifacts (dist-scalprum)
5. **Reports comprehensive summary** with success rates

## Architecture

Based on POC from PR redhat-developer#4523 but modernized:
- Uses @red-hat-developer-hub/cli-module-install-dynamic-plugins instead of Python script
- TypeScript implementation with proper types
- Integrated into existing Playwright project structure
- Runs in showcase-sanity-plugins nightly job

## Performance

- ~3 minutes for plugin extraction (first run)
- ~2 seconds for backend startup validation
- Subsequent runs are faster (cached plugins)

## Dependencies

- @red-hat-developer-hub/cli-module-install-dynamic-plugins@0.2.0
- @backstage/backend-test-utils (for startTestBackend)
- @backstage/plugin-catalog-backend
- @backstage/plugin-scaffolder-backend

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add explicit type annotation to require() call for better type safety.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implements review findings from PR redhat-developer#4967 code review:

**Correctness fixes:**
- Fix test.beforeAll signature to use test.info() (follows smoke-test.spec.ts pattern)
- Remove unused manifestPath variable in plugin-dynamic-loading.spec.ts

**Code organization:**
- Extract DEFAULT_PACKAGES_PATH constant to avoid duplication
- Move CONFIG_OVERRIDES to module scope for better reusability
- Remove unused catalog-index-parser.ts (can be recreated when needed)

**Type safety:**
- Import and use BackendFeature type for require() calls instead of any
- Improves type safety when loading plugin modules

**Developer experience:**
- Improve console.warn message to explain impact of missing _nodeModulePaths

All changes are low-risk refactorings that improve code quality without
changing behavior. Type checking passes with no errors.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Based on final code review feedback:

**Documentation improvements:**
- Clarify default.packages.yaml is only used for local testing
- Add comment explaining empty plugins config triggers catalog index extraction
- Expand KNOWN_FAILURES documentation with reasons for each exclusion
- Add JSDoc explaining purpose of KNOWN_FAILURES set

**Why these changes:**
- Makes it clearer when mock file vs CI-injected file is used
- Documents non-obvious behavior (empty config → index extraction)
- Helps future maintainers understand if failures can be re-enabled
- All changes are documentation-only, no behavior changes

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
gustavolira and others added 6 commits June 19, 2026 16:42
Fixes all eslint errors reported in CI:

**plugin-loader.ts:**
- Change LoadedPlugin.feature type from any to BackendFeature
- Fix eslint-disable comment for @typescript-eslint/no-require-imports
- Rename CONFIG_OVERRIDES to configOverrides (camelCase)
- Replace 'any' with 'unknown' in buildMergedConfig

**plugin-dynamic-loading.spec.ts:**
- Rename CORE_FEATURES to coreFeatures (camelCase)
- Remove explicit 'any[]' type annotation (inferred correctly)

**plugin-sanity-check.spec.ts:**
- Add eslint-disable comments for __filename/__dirname (ESM compat)
- Add eslint-disable comment for DEFAULT_PACKAGES_PATH (path convention)

All changes improve type safety while maintaining functionality.
Type checking and linting now pass with 0 errors.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…lugin tests

Improves documentation to address Qodo review feedback about test design.

**Context:**
Qodo flagged that plugin-sanity-check.spec.ts doesn't actually resolve packages,
calling it a bug. This is intentional design - we have TWO complementary tests:

1. plugin-sanity-check.spec.ts (LIGHTWEIGHT ~seconds)
   - Fast format validation
   - Catches YAML/config errors
   - Does NOT download/load plugins

2. plugin-dynamic-loading.spec.ts (COMPREHENSIVE ~3 min)
   - Downloads from catalog index
   - Actually loads plugins with startTestBackend
   - Validates runtime behavior

**Changes:**
- Expanded JSDoc headers explaining the two-test architecture
- Added inline comments explaining why format-only validation is intentional
- Clarified what each test catches vs doesn't catch
- Cross-referenced between the two test files

This makes the design intent explicit and prevents future confusion about
why plugin-sanity-check.spec.ts doesn't load plugins.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Applies Prettier formatting to plugin test files to pass CI checks.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removes plugin-sanity-check.spec.ts (depends on default.packages.yaml which
doesn't exist in RHDH repo) and keeps only plugin-dynamic-loading.spec.ts
which reads from catalog index and validates all plugins.

**CI Integration:**
- Test runs in showcase-sanity-plugins namespace (already configured)
- Uses PW_PROJECT_SHOWCASE_SANITY_PLUGINS Playwright project
- Executed by run_sanity_plugins_check() in ocp-nightly.sh
- Runtime: ~3 minutes for full plugin validation

**What the test does:**
1. Downloads all plugins from CATALOG_INDEX_IMAGE via install-dynamic-plugins
2. Loads backend plugins and validates they have valid exports
3. Starts test backend with startTestBackend to verify plugins work
4. Validates frontend plugins have required bundle artifacts

**Deployment:**
- Namespace: showcase-sanity-plugins (fixed)
- Values file: diff-values_showcase-sanity-plugins.yaml
- Merged with base values during deployment
- Uses same infrastructure as other showcase namespaces

This completes RHIDP-13508 implementation - comprehensive plugin sanity
check without cluster dependency, running in nightly CI.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…c-loading test

Adds @backstage/plugin-catalog-backend and @backstage/plugin-scaffolder-backend
as devDependencies to resolve import errors in plugin-dynamic-loading.spec.ts.

**Root Cause:**
The test imports catalogPlugin and scaffolderPlugin to use as core features
when starting the test backend with startTestBackend(), but these packages
were not declared in e2e-tests/package.json, causing:
  Error: Cannot find package '@backstage/plugin-catalog-backend'

**Dependencies Added:**
- @backstage/plugin-catalog-backend@3.5.0 (matches packages/backend version)
- @backstage/plugin-scaffolder-backend@3.3.0 (matches packages/backend version)

**Verification:**
- TypeScript compilation passes
- yarn install completes successfully
- Versions match those used in packages/backend/package.json for consistency

This fixes the CI failure in e2e-ocp-helm job where the showcase namespace
tests failed due to missing dependencies.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Adds ESM compatibility polyfill for __dirname in plugin-dynamic-loading.spec.ts
to fix ReferenceError when calling patchModuleResolution().

**Error:**
  ReferenceError: __dirname is not defined in ES module scope
  at patchModuleResolution(join(__dirname, "..", "..", "node_modules"))

**Fix:**
- Import fileURLToPath and dirname from Node.js path/url modules
- Define __filename and __dirname polyfills at module scope
- Add eslint-disable comments for naming-convention (matches pattern from plugin-sanity-check.spec.ts)

This is the standard ESM compatibility pattern used across e2e tests.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

Adds plugin-dynamic-loading.spec.ts to testIgnore for the showcase project
to prevent it from running in PR checks (e2e-ocp-helm job).

**Problem:**
The test was running in the showcase project during e2e-ocp-helm PR checks,
but it requires:
1. CATALOG_INDEX_IMAGE environment variable
2. install-dynamic-plugins CLI to download plugins
3. ~3 minutes execution time
4. Cluster deployment (not available in showcase namespace setup)

**Why this test shouldn't run in showcase:**
- showcase project runs in e2e-ocp-helm (PR checks)
- This test is designed for showcase-sanity-plugins (nightly only)
- PR checks should be fast (<10 min), this test takes ~3 min alone
- Test requires specific deployment setup not present in showcase

**Where the test SHOULD run:**
- showcase-sanity-plugins project (configured in playwright.config.ts)
- Executed by run_sanity_plugins_check() in ocp-nightly.sh
- Only runs in e2e-ocp-helm-nightly job (nightly, not PR checks)

This ensures the test only runs in the appropriate environment where
the infrastructure supports plugin extraction and validation.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

The container image build and publish workflows were skipped (either due to [skip-build] tag or no relevant changes with existing image).

@sonarqubecloud

Copy link
Copy Markdown

@gustavolira

Copy link
Copy Markdown
Member Author

/test e2e-ocp-helm-nightly

@openshift-ci

openshift-ci Bot commented Jun 20, 2026

Copy link
Copy Markdown

@gustavolira: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm-nightly 60a37f4 link false /test e2e-ocp-helm-nightly

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant