Skip to content

test: Use yarn ocap for e2e tests #268

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.

Sign up for GitHub

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

Jump to bottom
Merged
grypez merged 14 commits into from
Dec 6, 2024
Merged

test: Use yarn ocap for e2e tests #268

grypez merged 14 commits into from
Dec 6, 2024

Conversation

@grypez
Copy link
Contributor

@grypez grypez commented Dec 5, 2024 •
edited
Loading

Changes

@ocap/extension

  • Moves e2e test logic to the test-e2e-ci.sh shell script.
  • Uses ocap bundle and ocap serve to setup the e2e tests.

@grypez grypez force-pushed the grypez/cli-everywhere branch from eb712d8 to d19a668 Compare December 6, 2024 14:03
@grypez grypez force-pushed the grypez/cli-integrate-e2e-tests branch from b40a7d6 to ed546ba Compare December 6, 2024 14:08
Base automatically changed from grypez/cli-everywhere to main December 6, 2024 14:45
@grypez grypez force-pushed the grypez/cli-integrate-e2e-tests branch from ed546ba to 5371e83 Compare December 6, 2024 14:47
@grypez grypez marked this pull request as ready for review December 6, 2024 16:38
@grypez grypez requested a review from a team as a code owner December 6, 2024 16:38
sirtimid
sirtimid reviewed Dec 6, 2024
View reviewed changes
packages/extension/package.json Outdated
"test:verbose": "yarn test --reporter verbose",
"test:watch": "vitest --config vitest.config.ts",
"test:e2e": "playwright test",
"test:e2e": "yarn start:server & playwright test",
Copy link
Contributor

@sirtimid sirtimid Dec 6, 2024

Choose a reason for hiding this comment

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

But does the server close when the tests end? Was there an issue with playwrights webServer option?

Copy link
Contributor

@sirtimid sirtimid Dec 6, 2024

Choose a reason for hiding this comment

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

Ah I saw the hangup, not ideal tbh

Copy link
Contributor Author

@grypez grypez Dec 6, 2024

Choose a reason for hiding this comment

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

yes, the issue was playwright not noticing ocap serve is up when using the webServer command and just hanging waiting for it to start listening. I tried to notify of listening by sending 200 on empty / ping request but without success; so it seems to me playwright is not querying the server to find out if it's up but just waiting for the launch process to exit. the issue is the start:server command never returns control to playwright, and I would need to write a background process w/ disk state to do so. instead I opted to just run in a different shell process and auto hangup

Copy link
Contributor Author

@grypez grypez Dec 6, 2024

Choose a reason for hiding this comment

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

#226 asks about a daemon the cli can start / stop; given some time to implement a daemon, a stateful server would be an easy add-on. Then I could make the ocap serve command exit once the server is up instead of holding open while the server is running. In the mean time... 'not ideal' beats 'not implemented' 🙃

rekmarks
rekmarks requested changes Dec 6, 2024
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should remove the hangup options (and withTimeout etc.) and just do this instead: #270

To the extent that we can avoid relying on wall clock timers, we should do so.

rekmarks and others added 2 commits December 6, 2024 10:40
@rekmarks
test(extension): Refactor e2e test script (#270)
032b36d 
Refactors the e2e test invocation to avoid a timed hangup of the server.
We still start the server in parallel with Playwright, but we always
close the server by killing its process after the tests end.
@grypez
remove unused hangup functionality
2f36095
rekmarks
rekmarks reviewed Dec 6, 2024
View reviewed changes
rekmarks
rekmarks approved these changes Dec 6, 2024
View reviewed changes
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@grypez grypez merged commit 2ad3cdc into main Dec 6, 2024
15 checks passed
@grypez grypez deleted the grypez/cli-integrate-e2e-tests branch December 6, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sirtimid sirtimid sirtimid left review comments

@rekmarks rekmarks rekmarks approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@grypez @sirtimid @rekmarks