Skip to content

chore: update dependencies#13

Closed
demurgos wants to merge 1 commit intobcoe:masterfrom
demurgos:update-deps
Closed

chore: update dependencies#13
demurgos wants to merge 1 commit intobcoe:masterfrom
demurgos:update-deps

Conversation

@demurgos
Copy link
Contributor

This commit updates the dependencies to their latest version.

The only difference is a different expected output for the integration tests.

All files | 91.67 | 100 | 80 | 91.67 | |
normal.js | 85.71 | 100 | 50 | 85.71 | 14,15,16 |
timeout.js | 100 | 100 | 100 | 100 | |
------------|----------|----------|----------|----------|----------------|`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes expected?

Copy link
Contributor Author

@demurgos demurgos Jul 30, 2018

Choose a reason for hiding this comment

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

I had these results even without updating the dependencies, so I assumed that the change was caused by simply having a more recent version of Node.

By looking into it deeper, you are right that it reveals an issue. The functions in fixtures/normal are counted as 0/1 instead of 1/2. The apple function is not detected as a function.

I should probably revert the change for this file: the tests were already failing before updating the dependencies and there's no CI anyway. What do you think?

@demurgos demurgos mentioned this pull request Aug 2, 2018
@demurgos demurgos closed this Aug 4, 2018
@demurgos demurgos deleted the update-deps branch August 4, 2018 15:34
@demurgos
Copy link
Contributor Author

demurgos commented Aug 7, 2018

I isolated the example in a minimal setting. I currently get odd coverage results (not all the functions are matched) that may be related to the changes here. I need to check if there's an issue with the way I test it or if it's a Node/V8 issue.

@demurgos
Copy link
Contributor Author

demurgos commented Aug 7, 2018

Ok, I managed to get the right order of invocations to get V8 to return the coverage for normal.js with all the functions. I'm now check with v8-to-istanbul if it is properly converted. I was also confused by the results returned by V8 (I couldn't find any doc), but I seem to get it now.

Edit: V8 results: https://gist.github.com/demurgos/deb05f530f9f9f8e2542e7284df9de8f

@demurgos
Copy link
Contributor Author

demurgos commented Aug 11, 2018

My refactored v8-to-istanbul can now properly handle the functions (and statements) in normal.js.

Remaining work:

  • support branches
  • integrate my changes into c8

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.

2 participants