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

Add test for options to babel-cli #5398

Closed
wants to merge 1 commit into from

Conversation

outsideris
Copy link
Contributor

@outsideris outsideris commented Mar 1, 2017

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

It's tests for babel-cli's options, see the coverrage.

@outsideris outsideris changed the base branch from 7.0 to master March 4, 2017 18:08
@babel-bot
Copy link
Collaborator

Hey @outsideris! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@outsideris
Copy link
Contributor Author

I'm stuck here.

  1. Node v4 doesn't support --inspect. How can I handle it?
  2. The tests were passed in Node v7, but nyc which is for coverage looks like doesn't support debug options.

Also, I tried to add tests for --d, --debug and --debug-brk. But it is hard to make tests for them because these flags are hanging in debug console.

@xtuc
Copy link
Member

xtuc commented Mar 5, 2017

If the test can't run on Node v4 I would skip it, like:

it('foo', function() {
  if (getMajorVersion(process.version) === 4) {
    this.skip();
  }

});

@xtuc xtuc changed the title Add test for options to bable-cli Add test for options to babel-cli Mar 5, 2017
@babel-bot
Copy link
Collaborator

Hey @outsideris! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@babel-bot
Copy link
Collaborator

Hey @outsideris! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@codecov
Copy link

codecov bot commented Mar 12, 2017

Codecov Report

Merging #5398 into 7.0 will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #5398      +/-   ##
==========================================
+ Coverage   85.24%   85.29%   +0.05%     
==========================================
  Files         284      284              
  Lines        9958     9958              
  Branches     2780     2780              
==========================================
+ Hits         8489     8494       +5     
+ Misses        968      963       -5     
  Partials      501      501
Impacted Files Coverage Δ
packages/babel-helper-call-delegate/src/index.js 64% <0%> (-4%) ⬇️
packages/babel-traverse/src/path/modification.js 72.11% <0%> (-0.97%) ⬇️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️
packages/babel-traverse/src/path/context.js 85.34% <0%> (-0.87%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.21% <0%> (-0.43%) ⬇️
packages/babel-cli/src/babel-node.js 68.08% <0%> (+21.27%) ⬆️

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 070662e...1cee0e9. Read the comment docs.

@outsideris
Copy link
Contributor Author

I removed a test for a debug option, because there is an error with nyc.

@@ -174,7 +174,9 @@ fs.readdirSync(fixtureLoc).forEach(function (binName) {
opts.inFiles[".babelrc"] = helper.readFile(babelrcLoc);
}

it(testName, buildTest(binName, testName, opts));
if (testName !== "flag-dashed-inspect" || !process.version.match(/^v[045]/)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to exclude the test for --inspect in Node v0.12, v4 and v5.
I doesn't like this workaround, but I couldn't find better way.

Copy link
Member

Choose a reason for hiding this comment

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

We recently landed #5765, which allows you to specify a minimum node.js version via minNodeVersion

@existentialism existentialism added the PR: Docs 📝 A type of pull request used for our changelog categories label May 31, 2017
@existentialism
Copy link
Member

@outsideris sorry for the delay on this, but let's revive this as we landed #5785 and should probably get some test coverage around this.

@xtuc xtuc added area: tests and removed PR: Docs 📝 A type of pull request used for our changelog categories labels May 31, 2017
@xtuc
Copy link
Member

xtuc commented May 31, 2017

@existentialism I changed the label because you set (by mistake?) the documentation one.

@outsideris outsideris changed the base branch from master to 7.0 June 11, 2017 12:41
@outsideris
Copy link
Contributor Author

I'm trying to fix it and change the base branch to 7.0.
However although I specify minNodeVersion, it failed in Node v4
I missed something? @existentialism

@danez danez closed this Aug 31, 2017
@danez danez reopened this Aug 31, 2017
@danez danez changed the base branch from 7.0 to master August 31, 2017 18:24
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 15, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4907/

@jridgewell
Copy link
Member

I think minNodeVersion only work in fixture tests, not babel-cli tests.

@outsideris
Copy link
Contributor Author

So, how can I solve it?

@nicolo-ribaudo nicolo-ribaudo self-assigned this Feb 14, 2019
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: tests outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants