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

chore: warns the user appropriately for non-existent files #182

Merged
merged 7 commits into from
Jul 9, 2019

Conversation

jamesgeorge007
Copy link
Contributor

@jamesgeorge007 jamesgeorge007 commented Jun 22, 2019

Fixes #181

Shows up a warning rather than throwing the error.

start.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Would you mind applying the suggestion and adding a unit test?

@mcollina
Copy link
Member

You can mock module.exports.stop and verify it's getting called.

@jamesgeorge007
Copy link
Contributor Author

@mcollina It seems that 4 test cases are failing.

How should be the following error message be refactored 🤔

t.ok(/Cannot find module.*not-found/.test(err.message), err.message)

@mcollina
Copy link
Member

You need to change

function stop (error) {
so that it accept a second parameter (a string), and you print that string in stop if the error is not provided. Then, you need to change your PR so that the message is passed as a second argument instead.

start.js Outdated Show resolved Hide resolved
@jamesgeorge007
Copy link
Contributor Author

@xtx1130 Can you draw a better picture regarding the failing tests and how should it be fixed up 🤔

@xtx1130
Copy link
Contributor

xtx1130 commented Jul 1, 2019

@jamesgeorge007 I have got wrong, you need to accept mcollina's suggest, add another param to ensure stop support warning, such as :

function stop (error, warn) {
  if (error) {
    console.log(error)
    process.exit(1)
  }
  if (warn) {
   console.log(warn)
  }
  process.exit()
}

And you need to change:

return module.exports.stop(`${opts._[0]} doesn't exist within ${process.cwd()}`)

to

return module.exports.stop('', `${opts._[0]} doesn't exist within ${process.cwd()}`)

The test case should change to:

test('should throw on file not found', t => {
  t.plan(1)

  const oldStop = start.stop
  t.tearDown(() => { start.stop = oldStop })
  start.stop = function (err, warn) {
   // test case changes
    t.ok(/.*not-found doesn't exist within/.test(warn), warn)
  }

  const argv = [ '-p', getPort(), './data/not-found.js' ]
  start.start(argv)
})

@xtx1130
Copy link
Contributor

xtx1130 commented Jul 2, 2019

@jamesgeorge007 There still have some test cases you need to deal with. You can run npm run test in your local environment to make sure the test cases is correct.

@jamesgeorge007
Copy link
Contributor Author

@xtx1130 I'm not able to figure out what's going wrong with the currently failing tests. Any sort of help is appreciated 👏

start.js Outdated Show resolved Hide resolved
@jamesgeorge007
Copy link
Contributor Author

cc @xtx1130

test/start.test.js Outdated Show resolved Hide resolved
start.js Outdated Show resolved Hide resolved
remove eslint disable comment
test/start.test.js Show resolved Hide resolved
test/start.test.js Show resolved Hide resolved
test/start.test.js Show resolved Hide resolved
start.js Show resolved Hide resolved
@jamesgeorge007
Copy link
Contributor Author

cc @mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina mcollina merged commit 53c4df7 into fastify:master Jul 9, 2019
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.

Throws error on providing a file that doesn't exist with start command
4 participants