Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Return correct exit code when server fails to start #851

Merged
merged 2 commits into from
Aug 15, 2017

Conversation

michalholasek
Copy link
Contributor

🚀 Why this change?

This PR fixes the issue with wrong CLI exit code when running Dredd with incorrect argument to --server command (reported in #818).

📝 Related issues and Pull Requests

Closes #838

✅ What didn't I forget?

  • To write docs - N/A
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@michalholasek michalholasek force-pushed the michalholasek/cli-exit-code-regression branch from 9b4dd01 to fdb9118 Compare August 9, 2017 16:22
@honzajavorek
Copy link
Contributor

Failing in all builds:

  Babysitting Child Processes
    when gracefully terminated by childProcess.signalTerm()
      process without support for graceful termination
        ...

TypeError: Cannot read property 'indexOf' of undefined

@michalholasek michalholasek force-pushed the michalholasek/cli-exit-code-regression branch from fdb9118 to 3576ec9 Compare August 9, 2017 17:44
@michalholasek
Copy link
Contributor Author

Shit

@honzajavorek Fixed.

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

I like the changes 👍 Great job. I wonder what your replies to my comments would be, otherwise I'd approve this.

@serverProcess.on 'error', (error) =>
logger.error('Command to start backend server process failed, exiting Dredd', error)
@_processExit(2)
@serverProcess.on 'spawnerror', (err) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized we probably want to do this in case of any kind of error happening to the backend server child process. If the process errors, Dredd cannot ensure the testing will make sense, and it should stop testing and terminate. So I propose we actually keep @serverProcess.on 'error' here. What do you think?

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 have no problem with that, we can differentiate between different type of runtime errors in single error handler hub.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reflected in ab90347.

@@ -90,6 +90,26 @@ describe 'CLI - Server Process', ->
it 'should exit with status 0', ->
assert.equal dreddCommandInfo.exitStatus, 0

describe 'When it fails to start', ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized there's one more side to the problem: The process starts, but Dredd isn't able to connect to it using HTTP. Looking into coverage, this scenario seems to be untested:

image

transaction-runner.coffee


I hope it would still work as expected (Dredd fails all tests and it ends with exit code 1), but it would be awesome if we had a test for that as well. What do you think?

It should be fairly easy to test, with the same approach, just spawning an existing script, which hangs the same way as server would do (endless setTimeout loop?), but which doesn't handle HTTP in any way. Maybe there's even already a script like that in test/fixtures, I haven't take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I'll have a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

My attempt is in #862. If the section screenshoted above won't get green, then the test is useless and my PR should get closed.

@michalholasek michalholasek force-pushed the michalholasek/cli-exit-code-regression branch 2 times, most recently from ab90347 to 6e6c23c Compare August 14, 2017 16:12
Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

🚀

@michalholasek michalholasek merged commit 1693d1c into master Aug 15, 2017
@honzajavorek honzajavorek deleted the michalholasek/cli-exit-code-regression branch August 15, 2017 12:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants