Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(adapter): handle Playwright & Stencil configs in different dirs #39

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

tanner-reits
Copy link
Contributor

What is the current behavior?

The adapter only expects the Stencil config and Playwright config to be sibling files. This prevents tests from working with architectures that might have tests and test-related files in a sub-directory

GitHub Issue Number: N/A

What is the new behavior?

Replaces glob with find-up to search for the Stencil config file in the current and all ancestor directories. This way tests can live and be executed in architectures that might place the Playwright config at a lower directory level.

Documentation

N/A

Does this introduce a breaking change?

  • Yes
  • No

Testing

Updated automated tests.

Tested new behavior in a Stencil component starter in a few ways:

  1. Moving the Playwright test files and config file to a nested tests dir (i.e. like src/tests). Then cd to this dir, and run npx playwright test.
  2. Moving the Playwright test files and config file to a nested tests dir (i.e. like src/tests). From the root of the project, run npx playwright test -c <path_to_playwright_config>
  3. Moving the Playwright test files and config file to a nested tests dir (i.e. like src/tests). Then cd to a directory lower than the test directory (i.e. cd src/tests/sub-dir) and run npx playwright test -c ../playwright.config.ts

Other information

@tanner-reits tanner-reits requested a review from a team as a code owner April 12, 2024 19:47
const stencilConfigPath = globSync(`${process.cwd()}/stencil.config.{ts,js}`)?.[0];
// Find the Stencil config file in either the current directory, or the nearest ancestor directory.
// This allows for the Playwright config to exist in a different directory than the Stencil config.
const stencilConfigPath = await findUp(['stencil.config.ts', 'stencil.config.js']);
Copy link
Member

Choose a reason for hiding this comment

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

While the name stencil.config.ts is what we use in almost all examples, it would be technically possible to to name the config file differently which would cause confusion. Should we note this limitation in the README.md and clearly state that Stencil config files must follow this name pattern?

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I think either is of those 2 options is acceptable here 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll list this as a limitation in the readme & docs for now. Only other solutions that come to mind would be:

  • A CLI for the adapter that would allow you to specify a path to the Stencil config and invoke the Playwright CLI under the hood
  • A process.env variable for the Stencil config path.

I think both of those are a bit overkill/out-of-scope for now, though.

@tanner-reits
Copy link
Contributor Author

@rwaskiewicz @christian-bromann Gonna update the readme/docs separately from this. Turns out the readme didn't get the setup instruction updates I made on the docs before we released so I'm just gonna take care of all that together.

@tanner-reits tanner-reits added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit ead95cd Apr 18, 2024
3 checks passed
@tanner-reits tanner-reits deleted the tr/handle-nested-configs branch April 18, 2024 15:22
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.

3 participants