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

Tests improve coverage #7

Merged
merged 15 commits into from
Feb 4, 2022
Merged

Tests improve coverage #7

merged 15 commits into from
Feb 4, 2022

Conversation

jankapunkt
Copy link
Member

@jankapunkt jankapunkt commented Jan 20, 2022

Intends to improve test coverage to such an extend, that we can safely refactor code to full ES6 without regressions

Also found and fixed additional issues:

  • removed old unsused gulp files
  • bas64 support for EJSON added (uses the same package as React Native), also browser compatible
  • EJSON.equals fails to compare Array and Object if first param is Object and second one is Array this is actually an issue with the original EJSON code and I already filed a bug to the Meteor project
  • added unit tests for selector.compileSelector and selector.compileSort
  • compileSelector used in $where a fallback if argument is not a function, that relied on the Function constructor, which is replaced now with an arrow function closure
  • added unit tests for all transaction types

What remains to be done but is out of scope for this PR:

  • testing integration with RN and RN-Meteor
  • moving all code to es6; remove decaffeinate autocode (suggested by commets)
  • update lodash and eventemitter3 (vulneraibilites!) but that requires careful crafting as they have partially different apis (two/three major version differences)
  • add GitHub CodeQL to CI to find potential vulnerable code definitions not picked up by tests or linter

current coverage (updated on each push):

--------------------------------|---------|----------|---------|---------|---------------------
File                            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s   
--------------------------------|---------|----------|---------|---------|---------------------
All files                       |   91.86 |    90.73 |   94.11 |   92.04 |                     
 EJSON.js                       |   98.91 |    98.49 |     100 |     100 | 140,190             
 MemoryDb.js                    |   94.93 |    88.23 |      90 |    94.8 | 32-36,59,68         
 NullTransaction.js             |     100 |      100 |     100 |     100 |                     
 ReadOnlyTransaction.js         |     100 |      100 |     100 |     100 |                     
 ReadTransaction.js             |     100 |      100 |     100 |     100 |                     
 SynchronousWriteTransaction.js |     100 |      100 |     100 |     100 |                     
 WithObservableReads.js         |   92.85 |    83.33 |   85.71 |   92.85 | 30,60-61            
 WithObservableWrites.js        |     100 |      100 |     100 |     100 |                     
 WithReactMixin.js              |    9.52 |        0 |       0 |    9.52 | 11-81,89-92         
 WithServerQuery.js             |   85.93 |       80 |   78.57 |   85.93 | 26,117-126          
 WriteTransaction.js            |   89.28 |    83.33 |     100 |   89.09 | 86-87,92-96         
 selector.js                    |   96.52 |     92.1 |     100 |   97.84 | 417,421-423,495,659 
 utils.js                       |   96.09 |    91.89 |     100 |   95.93 | 161,214,225,252,300 
--------------------------------|---------|----------|---------|---------|---------------------

@jankapunkt jankapunkt linked an issue Jan 20, 2022 that may be closed by this pull request
@jankapunkt jankapunkt marked this pull request as ready for review January 21, 2022 23:05
@jankapunkt
Copy link
Member Author

@TheRealNate did you get a chance to review this one?

@jankapunkt jankapunkt requested a review from diavrank February 3, 2022 08:20
Copy link

@TheRealNate TheRealNate left a comment

Choose a reason for hiding this comment

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

Hey @jankapunkt, sorry about the delay. This is clearly a lot of work! These look good to me, feel free to merge whenever erady.

@jankapunkt jankapunkt merged commit 1425623 into master Feb 4, 2022
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.

Restructure and improve Tests
2 participants