-
Notifications
You must be signed in to change notification settings - Fork 340
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
docs: clarify server shutdown #915
docs: clarify server shutdown #915
Conversation
See Start server to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @MikeMcC399!
You're welcome! If you feel that the topic of killing server processes needs to be followed up, then I suggest opening a new issue and/or taking the discussion to the Discord chat board. |
I think it's clear enough from the community discussion over there that it's a confusing issue. This PR should not have closed #360. |
You would like the ability to kill a server process and you would like that documented. I don't believe that re-opening #360 would serve that purpose well, however I'm happy to let @nagash77 comment on that and decide. This is a complex topic and to do it justice it should have a clear separate new issue. |
@karlhorky if you feel there is functionality missing that you feel would be a beneficial addition to Cypress please open a feature request ticket and our product team will review and give it consideration. |
My comment above does not have to do with missing behavior from Cypress. It has to do with the original reason for #360 existing - that it was unusual behavior that the process was not stopping (a bug). #360 was about a bug, not lacking documentation, as I believe @MikeMcC399 incorrectly interpreted. This bug will be a sharp edge that some of your users will run into, with (now that #360 was closed) no open issue mentioning it. This behavior feels like a bug. What I was suggesting in my comment above was documenting alternatives for these points already here in #915 (because @MikeMcC399 decided that this PR closes #360):
For 1., apparently using multiple jobs should be fairly straightforward according to @MikeMcC399. So it would be a documentation block showing multiple jobs. For 2., if you check out #360, there are two workarounds for how to run another step after the Cypress GitHub action (and maybe the Cypress team has other recommendations for different commands). This works around the weird behavior of the Cypress GitHub Action, and offers clear recourse to users having this problem. This should have been a part of #915, but since it wasn't, it should be a part of whatever PR closes #360. |
I'm sorry that you don't like what has been done here. When I considered suggesting closure I also looked at the example in #360 using I found the workflow https://github.com/bahmutov/cypress-grep-example/blob/main/.github/workflows/main.yml there and this no longer had a problem because the job had been split up, so the issue description no longer represented a reproducible example using a current version of the action. The most effective open issues are ones which are reproducible on current versions. |
🎉 This PR is included in version 5.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR closes #360 and modifies the README > Start server section.
When a server is started in a job, it continues running until the job completes. The shutdown of a server is not linked to the end of testing as such. The current text was misleading in this respect.
An example can be seen by viewing actions/workflows/example-start.yml, for example job 8915428534