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

Importing logger from componentParser.js cause parser.test.js to fail #1060

Closed
nbriannl opened this issue Feb 27, 2020 · 4 comments · Fixed by #1265
Closed

Importing logger from componentParser.js cause parser.test.js to fail #1060

nbriannl opened this issue Feb 27, 2020 · 4 comments · Fixed by #1265

Comments

@nbriannl
Copy link
Contributor

nbriannl commented Feb 27, 2020

Is your request related to a problem?

In MarkBind Version: 2.11.0, I have an issue when importing logger from componentParser.js cause parser.test.js to fail.
For example, calling it accordingly as here

const logger = require('../../../../util/logger');

will cause parser.test.js to fail as shown below.
image

As @ang-zeyu has pointed out, this occurs for parser.js as well, which is likely the reason logger isn't used in there as well.

The issue is that parser.test.js mocks the whole fs module, which causes the DailyRotateFile constructor to fail automatically in finding the directory.

Describe the solution you'd like

A simple fix would be to only mock the specific functions of fs relavant to parser.test.js

(Describe your proposed solution here.)

Describe alternatives you've considered

(Write your answer here.)

Additional context

(Write your answer here.)

@ang-zeyu
Copy link
Contributor

Occurs for parser.js as well, which is likely the reason logger isn't used in there as well.

Issue is that parser.test.js mocks the whole fs module, which causes the DailyRotateFile constructor to fail automatically in finding the directory.
A simple fix would be to only mock the specific functions of fs relavant to parser.test.js

Perhaps this issue could be renamed as such

Thereafter a new issue could be opened Standardise usage of console.log and logger.log, which would also move the logger file to the markbind lib

@nbriannl
Copy link
Contributor Author

nbriannl commented Mar 1, 2020

What shall i renamed the issue as?

Edit: Renamed

@nbriannl nbriannl changed the title Importing logger from componentParser.js cause parser.test.js to fail Mock specific functions of fs relavant to parser.test.js Mar 1, 2020
@nbriannl nbriannl changed the title Mock specific functions of fs relavant to parser.test.js Use both unmocked and mocked fs in parser.test.js Apr 20, 2020
@nbriannl nbriannl changed the title Use both unmocked and mocked fs in parser.test.js Importing logger from componentParser.js cause parser.test.js to fail Apr 20, 2020
@nbriannl
Copy link
Contributor Author

This behavior no longer appears due to the mocking of the entire logger module in #1117, However, it would be more ideal if we use mocked and unmocked instances of fs in appropriate sections.

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jun 16, 2020

Resolved by #1207

Yet to un-mock logger in Parser.test.js

@ang-zeyu ang-zeyu reopened this Jun 28, 2020
ang-zeyu added a commit that referenced this issue Jun 30, 2020
The markbind core package resides in /src/lib/markbind/ directory of
the markbind-cli package, while the vue components package resides in
frontend/components.

Let's move these into /packages/ for consistent placing of
markbind-cli's dependencies.

Let's also rename the packages as scoped packages to reflect the new
directory structure.

In addition, the core package relied on markbind-cli's logger
utilities. This also created some issues with unit tests where the node
'fs' module was mocked (#1060).

Let's restructure the logging utilities slightly, giving the core
package it's own logger utility, and allowing markbind-cli to override
this configuration so that the old behaviour from a user standpoint
is preserved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants