Using webpack with pino for serverside builds: #2

[joeyvandijk]

separate CLI features for pretty.js (bin.js) with default features (pretty.js), to be able to use webpack with (ES6) Node.js code in serverless environments like AWS Lambda.

[davidmarkclements]

just one change :)

[davidmarkclements]

we no longer need this check - safe to remove since we won't be requiring bin.js

[jsumners]

It looks like we are missing cli tests. Would you be willing add some?

[davidmarkclements]

when you say CLI - do you mean pretty.js tests?

[jsumners]

The tests in pretty.test.js all use the method exported by pino.js. Unless I skimmed over one?

[joeyvandijk]

@jsumners is it fine to change pino.pretty() with pretty.js directly? While I do not know what you want to unit-test as bin.js is the same as pretty.js but with the shebang.

[davidmarkclements]

ah right right - so what are you thinking for CLI test (also as a result of this, we should add CLI test to pino-sapling) - say using child_process to exec('pino') ?

[jsumners]

What we are missing is:

1. does the help output correctly with -h
2. does the -l switch work
3. does the -t switch work

Maybe I'm being pedantic. Or maybe they can be solved in another PR.

[davidmarkclements]

another way would be to dynamically set process.argv, and clear bin.js from require.cache before each test

[davidmarkclements]

@jsumners I think this is a larger discussion, one for pino-sapling in fact

@joeyvandijk if you want to get involved in writing CLI tests here, that'd be great (we'd use the pattern you set for pino-sapling) - if not I'm happy to accept without CLI tests

[joeyvandijk]

@davidmarkclements not having enough background with node-tap I have difficulty implementing the CLI-test. If you show me an example or way to check the process.argv inside the test (or do I need to mock it) I can check again if I can add those missing cli-tests.

All your requested changes are done, please let me know if I missed one.

[davidmarkclements]

that's great

so there's two ways to do it, - you either mock process.argv, process.stin and process.stdout and do delete require.cache[require.resolve('../bin.js')) in a tap.beforeEach() - this is non trivial or you use child_process (which I think is going to be better as a general pattern, more explicit easier to understand) - so let's go with using child_process instead

something like this

var test = require('tap').test
var execSync = require('child_process').execSync
var fs = require('fs')
test('-h flag outputs usage.txt', (t) => {
  var usage = fs.readFileSync('../usage.txt').toString()
  var proc = execSync('node ../bin.js -h')
  t.is(proc.stdout.toString(), usage)
  t.end()
})

Not that tried that, might be buggy, but that's the general idea.

For test purposes it's fine to use sync methods

[mcollina]

LGTM as there were no tests before on the CLI, my bad.

I think the permissions on pretty.js must be changed, as it's still executable.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.