#134 - Test all binary expression compiler #170
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Removed an unused import of: ParameterDeclaration Resolve neo-one-suite#144 BinaryExpressionCompiler.ts Added code for instanceOf BinaryExpressionCompiler.ts Added test for instanceof
In BinaryExpressionCompiler.test.js: Added labels for tests for easy mapping to tested feature Added tests for all current BinaryExpressions in BinaryExpressionCompiler.js - Skipped failing tests for unsupported or features with bugs EqualsEqualsKeyword (Issue neo-one-suite#170) Adjusted indenting for all tests to be the same In BinaryExpressionCompiler.js: Added Comments describing tested or not tested
…ll_BinaryExpressionCompiler adding files that were not added in last commit
dicarlo2
requested changes
May 26, 2018
| if (4 / 2 !== 2) { | ||
| throw 'Failure'; | ||
| } | ||
| if (! ((128 >> 2) == 32) ) { |
There was a problem hiding this comment.
nit: remove the extra space after !
There was a problem hiding this comment.
here and throughout the file
|
|
||
| test('(6 & 5) === 4 [AmpersandToken]', async () => { | ||
| await helpers.executeString(` | ||
| if (!(6 & 5) === 4) { |
There was a problem hiding this comment.
I think you need to add parentheses: !((6 & 5) === 4). Makes it clearer anyways. Or just use !==.
| `); | ||
| }); | ||
|
|
||
| test('(127 ^ -6) === -123 [CaretToken]', async () => { |
There was a problem hiding this comment.
Looks like this is failing in CI.
|
|
||
| test('! (0 === null) [EqualsEqualsEqualsToken]', async () => { | ||
| await helpers.executeString(` | ||
| if (0 === null ) { |
There was a problem hiding this comment.
nit: remove extra space before )
|
|
||
| test('0 === false [EqualsEqualsEqualsToken]', async () => { | ||
| await helpers.executeString(` | ||
| if ( 0 === false ) { |
There was a problem hiding this comment.
nit: remove extra spacing in parentheses
| @@ -91,9 +91,11 @@ export default class BinaryExpressionCompiler extends NodeCompiler< | |||
| const pushValueOptions = sb.pushValueOptions(options); | |||
| switch (kind) { | |||
| case SyntaxKind.EqualsToken: | |||
| // Tested | |||
There was a problem hiding this comment.
Not a huge fan of these comments, but since we can't really get code coverage reports it's probably better to leave them in.
…ll_BinaryExpressionCompiler
dicarlo2
approved these changes
Jun 7, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Create tests for all BinaryExpressions in BinaryExpressionCompiler.js
Description of the Change
In BinaryExpressionCompiler.test.js:
Added tests for all binary expressions.
Features that are not supported are skipped
Added tests for all Binary Expressions.
In BinaryExpressionCompiler.js:
Variations of an expression or default cases that are not tested are marked with a "Not Tested" comment.
Test Plan
These are the unit tests.
Alternate Designs
None, followed existing pattern for tests.
Benefits
Most all variations of binary expressions are covered.
Possible Drawbacks
More tests results in longer test times.
Applicable Issues
Created Issue#170 for EqualsEqualsToken
This resolves Issue #134