Skip to content

Conversation

rynop
Copy link
Contributor

@rynop rynop commented Apr 16, 2020

  • This is a small change

Description:

Sorry I didn't file an issue cuz this was such a small change.

When using mocha, and trying to log within a hook/test detox/src/utils/customConsoleLogger.js:: getOrigin() blows up because userCallsite.getFileName() is undefined and therefore path.relative() explodes.

Simple reproduce:

  before: async () => {
    console.log('**********BLAH');

PR is not the most elegant solution, but not sure there is much that can be done when using https://github.com/sindresorhus/callsites

Without this fix, I can not use Detox (and I want to, cuz really like it :)

@rynop rynop requested a review from rotemmiz as a code owner April 16, 2020 17:05
Copy link
Collaborator

@noomorph noomorph left a comment

Choose a reason for hiding this comment

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

@rynop, looks valid, but could you add a test for that ? unknown filename case somewhere around here:

console.__log('');
expect(bunyanLogger.mock).toHaveBeenCalledWith(ignored(), expect.stringContaining(expectedOrigin), ignored(), ignored());
});
it('should use relative file-name rather than absolute in origin', () => {
const expectedOrigin = `at ${path.normalize('src/utils/customConsoleLogger.test.js')}:`;

@d4vidi
Copy link
Collaborator

d4vidi commented Apr 19, 2020

I must emphasize what @noomorph is asking regarding tests. This is TDD code -- need to add a failing test that acknowledges this specific case.

Also, please don't use ? for the file name, but an <unknown> notation.

Other than that: thank you for this! 😄

@rynop
Copy link
Contributor Author

rynop commented Apr 20, 2020

Fixed up and test added!

@rynop rynop requested a review from noomorph April 20, 2020 00:52
@rynop rynop requested a review from noomorph April 20, 2020 15:44
@noomorph
Copy link
Collaborator

noomorph commented May 6, 2020

@rynop, I am sorry for the delay - the last update from you somehow slipped between the cracks, and I have not noticed it. I'll review your PR soon.

@rynop
Copy link
Contributor Author

rynop commented May 6, 2020

@noomorph no worries!

@noomorph noomorph merged commit 4692ff2 into wix:master May 9, 2020
@noomorph
Copy link
Collaborator

noomorph commented May 9, 2020

@rynop, thank you! Great job! 👍

@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
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.

3 participants