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: Port in use error handling (#767) #784

Closed
wants to merge 1 commit into from

Conversation

NikhilM98
Copy link
Contributor

Fixes issue #767

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

could we add a test, sth like this?

const http = require('http')
const server = http.createServer((req, res) => {
  res.end()
}).listen(3000, async () => {
  console.log('listening at 3000')

  // run test here

  server.close(() => {
    console.log('server stopped')
  })
})

Copy link
Member

@JasonEtco JasonEtco left a comment

Choose a reason for hiding this comment

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

This is looking good - I left one note, in addition to @gr2m's suggestion!

src/index.ts Outdated Show resolved Hide resolved
@NikhilM98
Copy link
Contributor Author

NikhilM98 commented Dec 11, 2018

Thanks, @gr2m for the suggestion. But I'm facing another problem while writing tests. I was trying something like

  describe('initilization', () => {
    it('should expect the correct error if port already in use', async () => {
      const http = require('http')
      const server = http.createServer().listen(3001, async () => {
        const testApp = createProbot({ port: 3001 })
        testApp.logger.error = jest.fn()
        testApp.start( () => {
          expect(testApp.logger.error.mock.calls[0]).toBe('Port:3001 is already in use. You can define the PORT environment variable into use a different port.')
          server.close()
        })
      })
    })
  })

But testApp.start is taking infinite time to proceed. I think I should not have called testApp.start directly. I'd be great if you can give me some advice.

@gr2m
Copy link
Contributor

gr2m commented Dec 12, 2018

I don’t know when I’ll have time myself and I’m not as familiar with Probots code base, please give me a few days. Or maybe someone else can beat me to helping you out :)

@NikhilM98 NikhilM98 changed the title [FIX] Port in use error handling (#767) fix: Port in use error handling (#767) Feb 13, 2019
@NikhilM98
Copy link
Contributor Author

@JasonEtco I've changed process.exit() to process.exit(1) as requested.
@gr2m I'm still facing issue with calling .start() function in test.js as described here

@gr2m
Copy link
Contributor

gr2m commented Feb 16, 2019

@NikhilM98 I don’t know what the problem could be out of my head, I’d need to spend some more time investigating and I don’t have the time right now. Maybe you or someone else finds out what is going on, if not I’ll get back to it eventually, but not anytime soon

@github-actions
Copy link

🎉 This issue has been resolved in version 9.11.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

None yet

3 participants