Skip to content

[INTERNAL] JSModuleAnalyzer: Remove invalid log usage#354

Merged
tobiasso85 merged 2 commits intomasterfrom
lbt-jsmoduleanalyzer-log-fix
Oct 18, 2019
Merged

[INTERNAL] JSModuleAnalyzer: Remove invalid log usage#354
tobiasso85 merged 2 commits intomasterfrom
lbt-jsmoduleanalyzer-log-fix

Conversation

@tobiasso85
Copy link
Copy Markdown
Contributor

@tobiasso85 tobiasso85 commented Oct 15, 2019

Change usage from non existing log.debug to log.verbose.

This code was not yet covered by tests. The new tests ensure that the code is run through and no error is thrown.

Change usage from non existing log.debug to log.verbose
@coveralls
Copy link
Copy Markdown

coveralls commented Oct 15, 2019

Coverage Status

Coverage increased (+0.08%) to 90.178% when pulling 4ce5d9d on lbt-jsmoduleanalyzer-log-fix into b2d8269 on master.

@RandomByte
Copy link
Copy Markdown
Member

Got introduced with #341

RandomByte
RandomByte previously approved these changes Oct 15, 2019
Copy link
Copy Markdown
Member

@codeworrior codeworrior left a comment

Choose a reason for hiding this comment

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

I would rather fix the existing test. The log lines ARE already covered by the EVOBundle test in JSModuleAnalyzer. But the test unfortunately totally stifles all errors.

The issue is caused by line https://github.com/SAP/ui5-builder/blob/master/test/lib/lbt/analyzer/JSModuleAnalyzer.js#L97,

In JSModuleAnalyzer test the analyzeModule function will fail if there
is an error.
@tobiasso85
Copy link
Copy Markdown
Contributor Author

I would rather fix the existing test. The log lines ARE already covered by the EVOBundle test in JSModuleAnalyzer. But the test unfortunately totally stifles all errors.

The issue is caused by line https://github.com/SAP/ui5-builder/blob/master/test/lib/lbt/analyzer/JSModuleAnalyzer.js#L97,

Good point! Addressed it with 4ce5d9d

});

// Note: this test still fails as the evo-style bundles don't declare the names of the
// contained raw-modules any longer.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Was this comment removed intentionally?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess, yes. Because this TODO was, what #341 wanted to addressed (migration of 3562114).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought this fix made the tests not fail anymore therefore I removed it

@tobiasso85 tobiasso85 requested a review from RandomByte October 16, 2019 13:41
@tobiasso85 tobiasso85 merged commit 75e27bf into master Oct 18, 2019
@tobiasso85 tobiasso85 deleted the lbt-jsmoduleanalyzer-log-fix branch October 18, 2019 05:36
d3xter666 pushed a commit to UI5/cli that referenced this pull request Sep 25, 2025
…i5-builder#354)

Change usage from non existing log.debug to log.verbose
In JSModuleAnalyzer test the analyzeModule function will fail if there
is an error.
d3xter666 pushed a commit to UI5/cli that referenced this pull request Sep 25, 2025
A new module lib/lbt/utils/parseUtils is introduced that exposes a parseJS
function and a Syntax object with all AST node types. The new module is
used in all places where JavaScript code is parsed (incl. tests).

The module uses espree v6 for parsing instead of esprima. This is the
maximum version that can be used without introducing breaking changes to
the tooling (higher versions of espree require higher node versions than
documented in our package.json).

The set of allowed parser options is limited to ease a later migration
to another parser.

Fixes SAP/ui5-tooling SAP/ui5-builder#354

Co-authored-by: Merlin Beutlberger <m.beutlberger@sap.com>
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.

4 participants