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

Ensure new runs by deleting current runner instance when server calls stop function #200

Conversation

gdavidkov
Copy link
Contributor

This pull request includes a small change to the Server class in the packages/server/src/Server.ts file. The change ensures that the runner is deleted when the server is closed to allow another run.

@gdavidkov gdavidkov changed the title Ensure new runs by deleting current runner instance when server calls stop functio Ensure new runs by deleting current runner instance when server calls stop function Oct 29, 2024
@gdavidkov
Copy link
Contributor Author

@kraenhansen Can you take a look at it? The problem I encounter is that sometimes the window is closed while the tests are being executed, and after that,t I cannot start a new run. I tried to stop and start the server but without luck. The error that I got is A run is already in progress.

@kraenhansen
Copy link
Owner

Thanks for mention - I don't know why I missed this PR initially 👍 The fix looks reasonable.

Ideally, I would like to add a failing test which gets solved by this fix, to avoid regressions 🤞
Would you be able to implement that test as well?

@kraenhansen kraenhansen self-assigned this Jan 28, 2025
@gdavidkov
Copy link
Contributor Author

Yes, I’d be happy to implement that test.

@gdavidkov
Copy link
Contributor Author

I fixed one more case where the client was disconnected while the tests were running.
There is still one failing test in the CLI package."

@kraenhansen
Copy link
Owner

kraenhansen commented Jan 30, 2025

Thanks a lot for your suggestions! The failing test in the CLI package had to do with the runner emitting an "end" event before a "start" event, this required quite a bit of refactoring in the resetting code.

I've also refactored the tests you added to use a single client written in TS - please have a look at my changes and add any comments you might have. If you approve, the tests are now passing and we can get this merged and released 👍

(thanks again for your contribution and patience on this).

@gdavidkov
Copy link
Contributor Author

Thanks for refactoring the tests to use a single TS client! I’ve reviewed your changes and everything looks good to me. I'm approving the PR—please go ahead and merge and release it.

@kraenhansen kraenhansen merged commit 53bac6e into kraenhansen:main Feb 4, 2025
1 check passed
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