test(@embark/core): add tests for logger module

[0x-r4bbit]

No description provided.

[ghost]

DeepCode's analysis on #4328fd found:

• ❌ 0 critical issues. ⚠️ 0 warnings and 2 minor issues. ✔️ 2 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.

---

☺️ If you want to provide feedback on our bot, here is how to contact us.

[0x-r4bbit]

Added another commit that refactors the module (hopefully for the better)

[michaelsbradleyjr]

Nice work! See review comments below re: some things I think need attention.

[emizzle]

Great job!!

Just a couple of small suggestions on the tests.

[emizzle]

Maybe we could also inspect the args coming through to the fake logFunction as well?

[0x-r4bbit]

This is already covered in the follow test

[emizzle]

I could definitely be wrong, but aren't there more args than just the single string parameter that is tested for in the next test?

[0x-r4bbit]

The following test checks if the logFunction, which is testLogFn gets called with the expected parameters. What we can't see is that logger actually performs a mutation on the input parameter of methods like info() and error().

It puts color codes around the given message, that's why the test checks against {color_code}input{color_code}.

Does that make sense? Or am I misunderstanding what you're asking for

[emizzle]

I really think we should not be reading the file system here and instead faking appendFileSync (or whatever async equivalent we decide to go with). This will require fs to be configurable on Logger (obviously why we should be using DI, but I digress), so maybe the easiest option is to tack on an fs property in the constructor:

class Logger {
  constructor(options) {
    // ...
    this.fs = options.fs || fs;
    // ...
  }
}

And then in the test setup:

const appendFileSync = sinon.fake(); // could be async version depending on if you implement michael's suggestion
const fs = { appendFileSync };
logger = new Logger({
  events: embark.events,
  logFunction: testLogFn,
  logFile: "test"
  fs
});

and then in the test:

logger.info('Some test log');
sinon.assert.calledWith(appendFileSync, "test", ... rest of params);

[ghost]

DeepCode's analysis on #2a5abe found:

• ❌ 0 critical issues. ⚠️ 0 warnings and 2 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.

---

☺️ If you want to provide feedback on our bot, here is how to contact us.

[michaelsbradleyjr]

Good work so far. I have some questions/comments below.

Also, this is the first time in awhile I've really looked hard at the logger and one thing that jumped out at me is that by pinning _logFunction to console.log it means that our error, warn, etc. output are all going to stdout rather than stderr.

In a terminal:

$ node -e 'console.log("hello")' 2>/dev/null
hello
$ node -e 'console.error("hello")' 2>/dev/null
$ node -e 'console.warn("hello")' 2>/dev/null
$ node -e 'console.debug("hello")' 2>/dev/null
hello
$ node -e 'console.trace("hello")' 2>/dev/null

Since it's standard behavior that some loglevels go to stderr instead of stdout it seems like we should probably have our logger work the same way when it comes to printing to the screen (it's fine that all of the levels write into one log file). Instead of only having _logFunction we could also have _warnFunction, _debugFunction, etc. all by default being set to their console.[level] counterparts. The functions we define in logger/src/index.js for info, warn, etc. could pass the desired _[level]Function as an argument to logFunction.

Maybe the behavior change re: stdout/stderr belongs in another PR instead of this one.

[0x-r4bbit]

I've updated the log APIs to take an optional callback parameter so we can be notified inside tests when a log method is done (instead of relying on e.g. fs.statSync with a timeout loop to determine when a logger has written to a log file yet). /cc @emizzle

[michaelsbradleyjr]

I wanted to use the improved logger (of this PR) and ran into some problems. I've got a branch in progress that I'll submit as a PR to this PR branch, but it's still WIP at the moment, will wrap it up tomorrow; definitely simpler than trying to explain what's wrong and how to fix it.

Short version: the origin calculation doesn't work if it's inside the async.cargo; also, each distinct message needs to be on its own line with timestamp, etc. otherwise it's very confusing to read.

See: test/core/logger...test/core/logger-mb

See: 2a5abe7

[michaelsbradleyjr]

Rebased and added a commit with several changes, please re-review. See: 2a5abe7.

[michaelsbradleyjr]

@emizzle I believe your change request is satisfied by my commit on this branch. See 2a5abe7. If so, can you clear your change request? I think this PR is otherwise good to go.