Skip to content

Improve tests for slang rttc #45

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

Improve tests for slang rttc #45

wants to merge 0 commits into from

Conversation

@ning-y
Copy link
Member

@ning-y ning-y commented May 26, 2018

Features

  • 100% coverage for src/slang/utils/rttc.ts
  • Better use of jest methods, toBeUndefined, toBeInstanceOf
  • Better mock Closure which uses the Closure constructor
  misc.ts            |    44.44 |        0 |        0 |    44.44 |... 48,49,51,53,59 |
  object.ts          |        0 |      100 |        0 |        0 |               3,4 |
 src/slang/utils     |    70.87 |    68.29 |       75 |    69.39 |                   |
  node.ts            |    33.33 |     7.14 |    44.44 |    33.33 |... 08,114,115,116 |
  rttc.ts            |      100 |      100 |      100 |      100 |                   |
---------------------|----------|----------|----------|----------|-------------------|

Test Suites: 10 passed, 10 total
Tests:       32 passed, 32 total
Snapshots:   184 passed, 184 total
Time:        5.612s

Issues fixed

@ning-y ning-y requested a review from remo5000 May 26, 2018 15:31
remo5000
remo5000 approved these changes May 27, 2018
View reviewed changes
src/slang/utils/rttc.ts
return isBool(value) ? undefined : new TypeError(node, '', 'boolean', typeOf(value))
default:
return
if ((operator === '+' || operator === '-') && !isNumber(value)) {
Copy link
Contributor

@remo5000 remo5000 May 27, 2018

Choose a reason for hiding this comment

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

I feel like the switch case used previously is clearer, although simplifying the logic would help with more operators

Copy link
Member Author

@ning-y ning-y May 27, 2018

Choose a reason for hiding this comment

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

So the thing is that with the switch case, in order to get 100% test coverage there would need to be a call to checkUnaryExpression(context, ''. value). Otherwise, the default case would not be covered by the tests.

So assuming we want 100% test coverage, it was either that, or to change the switch case to the if/else here. Since there isn't much sense in checking for an empty operator, I decided to go with the latter instead.

Copy link
Contributor

@remo5000 remo5000 May 27, 2018

Choose a reason for hiding this comment

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

Can the default case be used to test for an unspecified unary operator (e.g delete or typeof)? If so, it can be meaningfully used

Copy link
Member Author

@ning-y ning-y May 27, 2018

Choose a reason for hiding this comment

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

That sounds like a good idea, tbh.

Unspecified operators are currently checked for in the linting step at at noUnspecifiedOperators. Checking the operators again during runtime seems redundant at first, but I feel like it's extra safety without any real performance hits.

Copy link
Member Author

@ning-y ning-y May 27, 2018

Choose a reason for hiding this comment

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

@evansb Can we check, what's the best practice here?

ning-y added a commit that referenced this pull request May 27, 2018
@ning-y
Improve test coverage for slang's rttc (#45)
1559e07 
1. Had to make changes to src/mocks/context.ts in order to for the mock Closure
   to use the Closure constructor (our own typeOf uses typeof, which checks the
   constructor to determine object types).
2. To get 100% test coverage, there was a need to call the elaborate method of
   TypeError. Went ahead and replaced that with explain() since it's probably
   better than 'TODO'.
3. Best way to test the error messages was with snapshots, IMO.

Resolves #38.

* Fix typing

Typescript thinks that errors could possible be undefined.

* Add tests for valid binary <, >, <=, >=

Also fixed the test title ('<==|>==' -> '<=|>=')
@ning-y ning-y closed this May 27, 2018
@ning-y ning-y deleted the rttc-test branch May 27, 2018 05:36
@ning-y
Copy link
Member Author

ning-y commented May 27, 2018

Had to do some git stuff to get github to mark this PR as merged after merging on the CLI.

(refer to 1559e07 for the changes)

@ning-y ning-y restored the rttc-test branch May 27, 2018 05:39
@ning-y
Copy link
Member Author

ning-y commented May 27, 2018

Nevermind, it still marks this PR as closed instead of merged.

@ning-y ning-y deleted the rttc-test branch May 27, 2018 09:38
ning-y referenced this pull request May 27, 2018
@remo5000 @ning-y
Disallow unspecified operators (#30)
a5aae47 
* Add unspecified operators rule

* Remove strict equality

* Update error message slightly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remo5000 remo5000 remo5000 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ning-y @remo5000