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

Improve babel-generator's code coverage #5338

Merged
merged 1 commit into from Feb 25, 2017

Conversation

@alxpy
Copy link
Contributor

alxpy commented Feb 18, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Deprecations? no
Spec Compliancy? yes
Tests Added/Pass? yes
Fixed Tickets #5326
License MIT
Doc PR
Dependency Changes no

I worked on improvement babel-generators code coverage: added a couple of tests, removed the code that does not meet spec (ExportAllDeclaration has no exported field, proof) and I proposed to remove the obsolete code (7e540cd).

I see no reason to test this code, it is deleted in 7.0.

@mention-bot

This comment has been minimized.

Copy link

mention-bot commented Feb 18, 2017

@alxpy, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth, @hzoo and @cpojer to be potential reviewers.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 18, 2017

Codecov Report

Merging #5338 into master will increase coverage by 0.07%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5338      +/-   ##
==========================================
+ Coverage   89.44%   89.52%   +0.07%     
==========================================
  Files         204      204              
  Lines        9949     9944       -5     
  Branches     2689     2688       -1     
==========================================
+ Hits         8899     8902       +3     
+ Misses       1050     1042       -8
Impacted Files Coverage Δ
packages/babel-generator/src/generators/modules.js 100% <ø> (+3.44%)
...ages/babel-generator/src/generators/expressions.js 96.15% <ø> (+3.07%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07dd2b1...6bc7a49. Read the comment docs.

@@ -45,12 +45,6 @@ export function ExportAllDeclaration(node: Object) {
this.word("export");
this.space();
this.token("*");
if (node.exported) {

This comment has been minimized.

Copy link
@hzoo

hzoo Feb 22, 2017

Member

Ah yeah I was thinking of export * as ns from 'mod'; from https://babeljs.io/docs/plugins/transform-export-extensions/ but it's a different node - ExportNamespaceSpecifier ^

This comment has been minimized.

Copy link
@alxpy

alxpy Feb 22, 2017

Author Contributor

I was thinking it is ExportNamedDeclaration -- babel/babylon#367

This comment has been minimized.

Copy link
@hzoo

hzoo Feb 22, 2017

Member

You can plug it into https://astexplorer.net: * as ns is ExportNamespaceSpecifier

This comment has been minimized.

Copy link
@alxpy

alxpy Feb 22, 2017

Author Contributor

oh, yes, I watched, sorry... we just talked about different parts of the AST =)

@hzoo
hzoo approved these changes Feb 22, 2017
@xtuc
xtuc approved these changes Feb 22, 2017
Copy link
Member

xtuc left a comment

Nice work 👍

@@ -0,0 +1,7 @@
let a = do {

This comment has been minimized.

Copy link
@aaronang

aaronang Feb 23, 2017

Member

Given your test case, when it fails, you might not be sure that it is actually the DoExpression that is failing. So in this case, I would argue that we might want to go for a more isolated test like:

(do {})

@hzoo @xtuc What are your thoughts on this?

This comment has been minimized.

Copy link
@hzoo

hzoo Feb 25, 2017

Member

Sure - we can add ^ in this test as well

@hzoo hzoo merged commit 01918c6 into babel:master Feb 25, 2017
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 07dd2b1...6bc7a49
Details
codecov/project 89.52% (+0.07%) compared to 07dd2b1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
existentialism added a commit that referenced this pull request May 19, 2017
@lock lock bot added the outdated label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.