Skip to content

Conversation

@QuintonJason
Copy link
Contributor

Fix E2E Test Timeouts

Problem

Several E2E tests were failing due to 30-second timeouts, preventing reliable CI/CD execution:

  • pds-checkbox.e2e.ts
  • pds-modal.e2e.ts
  • pds-progress.e2e.ts
  • pds-dropdown-menu.e2e.ts
  • pds-tabs.e2e.ts
Header Header
Screenshot 2025-07-08 at 8 59 47 AM Screenshot 2025-07-08 at 8 57 56 AM

Solution

Implemented comprehensive timeout and stability improvements for E2E testing:

Configuration Updates:

  • Added jest.config.js with 60-second timeout for E2E tests
  • Created src/test-setup.js for global Jest configuration
  • Updated Stencil config browser settings for improved stability

Test Improvements:

  • Added await page.waitForChanges() after component setup to ensure proper hydration
  • Improved async handling patterns in failing E2E tests

Results

All 645 tests now pass (previously 5 failing)
All 77 test suites complete successfully
Total runtime: 39.3 seconds (improved performance)
Zero timeout failures

Testing

  • Full test suite passes locally
  • Previously failing E2E tests now complete within 60s limit
  • No regressions in existing functionality

@QuintonJason QuintonJason self-assigned this Jul 8, 2025
@github-actions github-actions bot added the package: core Changes have been made to the Core package label Jul 8, 2025
@netlify
Copy link

netlify bot commented Jul 8, 2025

Deploy Preview for pine-design-system ready!

Name Link
🔨 Latest commit 0e6e1b5
🔍 Latest deploy log https://app.netlify.com/projects/pine-design-system/deploys/686d257b2a9b3100082ed602
😎 Deploy Preview https://deploy-preview-477--pine-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@QuintonJason
Copy link
Contributor Author

@coderabbitai please review this PR.

@coderabbitai
Copy link

coderabbitai bot commented Jul 8, 2025

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@QuintonJason, I'll review this PR for you. Let me gather the necessary information about the changes.


🏁 Script executed:

#!/bin/bash
# Get PR information including title, description, and changed files
gh pr view 477 --json title,body,files,additions,deletions,changedFiles

# Get the diff for the PR
gh pr diff 477

Length of output: 12277


PR Review: E2E Test Timeout Fixes

Great work on addressing the E2E test timeout issues! This is a comprehensive and well-targeted solution that tackles the root cause of the failures. Here's my detailed review:

Strengths

  1. Comprehensive Approach: The PR addresses timeouts at multiple levels - Jest configuration, Stencil browser settings, and individual test improvements.

  2. Excellent Results: Going from 5 failing tests to 0 failures with improved performance (39.3s runtime) demonstrates the effectiveness of the solution.

  3. Proper Async Handling: The strategic addition of await page.waitForChanges() after component setup and interactions is exactly the right approach for Stencil E2E tests.

  4. CI/CD Friendly: The browser args (--no-sandbox, --disable-dev-shm-usage, --disable-gpu) are appropriate for headless CI environments.

🔧 Minor Suggestions for Optimization

  1. Jest Configuration Cleanup - The jest.config.js file has some redundant settings:
// Consider simplifying to focus on E2E-specific needs:
module.exports = {
  testTimeout: 60000,
  setupFilesAfterEnv: ['<rootDir>/src/test-setup.js'],
  testMatch: ['**/*.e2e.(js|ts)'], // More specific for E2E tests
  transform: {
    '^.+\\.(ts|tsx)$': 'ts-jest'
  },
  moduleFileExtensions: ['ts', 'js', 'json']
  // Coverage settings might not be needed for E2E tests
};
  1. Test Setup Simplification - The empty hooks in test-setup.js can be removed:
// Simplified version:
jest.setTimeout(60000);
  1. Variable Declaration Fix - Good catch on the let to const change in pds-checkbox.e2e.ts:
const checkbox = await page.find('pds-checkbox >>> input'); // ✅ Better than let

🎯 Code Quality Observations

  1. Consistent Pattern: The await page.waitForChanges() additions follow a consistent pattern across all affected test files.

  2. Stencil Best Practices: The browser configuration in stencil.config.ts follows Stencil's recommended settings for CI environments.

  3. Input Handling Improvement: The change from page.keyboard.type() to input.type() in pds-input.e2e.ts is a good optimization that's more direct and reliable.

📊 Test Coverage Impact

The changes maintain the same test coverage while improving reliability. The exclusion patterns in the Jest config properly avoid testing test files themselves.

🚀 Overall Assessment

This is a solid, well-executed fix that addresses a critical infrastructure issue. The solution is:

  • Targeted: Addresses the specific timeout problems
  • Comprehensive: Covers configuration, setup, and individual test improvements
  • Maintainable: Uses clear, consistent patterns
  • Proven: Demonstrates measurable improvement in test results

The PR is ready for merge with the minor optimizations being optional improvements rather than blocking issues.

Final Score: 9/10 - Excellent work on improving the E2E test infrastructure!

@QuintonJason QuintonJason marked this pull request as draft July 8, 2025 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core Changes have been made to the Core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants