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

Jasmine doesn't print all of its test output on Node 12 #768

Closed
3 tasks done
breautek opened this issue Jul 11, 2019 · 8 comments · Fixed by #774 or #783
Closed
3 tasks done

Jasmine doesn't print all of its test output on Node 12 #768

breautek opened this issue Jul 11, 2019 · 8 comments · Fixed by #774 or #783
Labels

Comments

@breautek
Copy link
Contributor

Bug Report

Problem

When running npm test, Jasmine does not print all of its output to terminal

What is expected to happen?

For jasmine to print all test output

What does actually happen?

See output below:

norman@norman-ThinkPad:~/development/cordova-android$ npm test

> cordova-android@8.1.0-dev test /home/norman/development/cordova-android
> npm run eslint && npm run cover && npm run java-unit-tests


> cordova-android@8.1.0-dev eslint /home/norman/development/cordova-android
> eslint bin spec test


> cordova-android@8.1.0-dev cover /home/norman/development/cordova-android
> nyc jasmine --config=spec/coverage.json

Started
..................................................................................F................................................................................................................................................................................................................--------------------------------|----------|----------|----------|----------|-------------------|
File                            |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------------------------|----------|----------|----------|----------|-------------------|
All files                       |     59.6 |    55.85 |    58.82 |    60.01 |                   |
 lib                            |     50.9 |    50.98 |    36.84 |    51.52 |                   |
  create.js                     |     50.9 |    50.98 |    36.84 |    51.52 |... 85,343,352,361 |

<Rest of test output has been omitted>

Jasmine has one failing test, but doesn't output failure details.

Information

The issue only occurs on Node 12. Running node 10 the tests runs and unit test failures is printed to terminal properly.
This could be more of a jasmine, or a rewire issue but I've isolated the issue to the run.spec.js unit test should print out usage and help. Commenting out that unit test fixes the jasmine output.

Command or Code

npm test while running Node 12.

Environment, Platform, Device & Versions

Node 12
Linux Ubuntu 18.04
cordova-android@8.1.0-dev (master at the time of writing)
9.0.0 (cordova-lib@9.0.1)

Checklist

  • I searched for existing GitHub issues
  • I updated all Cordova tooling to most recent version
  • I included all the necessary information above
@brodycj
Copy link
Contributor

brodycj commented Jul 12, 2019

Thanks @breautek, I see the same thing on my mac. If we would add Node.js 12, I tried in PR #770, this test failure does not seem to trigger a build failure on AppVeyor CI. (Travis CI also succeeds with the ignored failure on my own fork.)

I would highly recommend you consider trying out the more verbose test reporter, like I think we did in cordova-lib, cordova-common, and cordova-ios.

@breautek
Copy link
Contributor Author

I tried using jasmine-spec-reporter to see if the output will change, but it was like it was ignoring the reporter and was using the jasmine default.

I suspect it has something to do with NodeJS 12 + help() method using process.exit

@brodycj
Copy link
Contributor

brodycj commented Jul 12, 2019

I suspect it has something to do with NodeJS 12 + help() method using process.exit

Interesting, thanks. I hope someone will get a chance to investigate it further, does not sound so good.

I also hope someone will get a chance to look further into getting npm test to work with jasmine-spec-reporter. I think it can really help us investigate and understand this kind of a problem in the future.

I would like to express our appreciation so far for the efforts you have taken to understand, investigate, and resolve some recent issues.

@breautek
Copy link
Contributor Author

I think this issue is related to jhnns/rewire#167

In Node 10: We have...

norman@norman-ThinkPad:~/development/cordova-android$ nvm run 10
Running node v10.16.0 (npm v6.9.0)
> Object.keys(global)
[ 'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout' ]

And in Node 12...

Welcome to Node.js v12.4.0.
Type ".help" for more information.
> Object.keys(global)
[
  'global',
  'clearInterval',
  'clearTimeout',
  'setInterval',
  'setTimeout',
  'queueMicrotask',
  'clearImmediate',
  'setImmediate'
]

You'll notice that process is no longer in global. rewire had problems with other globals not working properly because of this exact same issue.

@brodycj
Copy link
Contributor

brodycj commented Jul 12, 2019

I suspect it has something to do with NodeJS 12 + help() method using process.exit

https://github.com/apache/cordova-android/blob/8.0.0/spec/unit/run.spec.js#L204-L207

You'll notice that process is no longer in global.

jhnns/rewire#167 (comment)

Argh!

It looks like they started discussing a solution in jhnns/rewire#167 (comment). Does this make sense?

I wonder if we should just wait for the to fix it on rewire, or if we should consider some kind of a temporary workaround in the test suite?

Any ideas or other input from @raphinesse?

@breautek
Copy link
Contributor Author

So I discovered that

run.__set__({ console: { log: logSpy, error: errorSpy }/*, process: procStub*/ });
run.__set__('process.exit', _ => null);
run.__set__('process.cwd', _ => '');
run.__set__('process.argv', ['','']);

appears to work. Instead of "rewiring" process itself seems to break on Node 12, rewiring members of process works.

@brodycj
Copy link
Contributor

brodycj commented Jul 12, 2019

Thanks @breautek, looks like you solved it (with a few minor lint issues). I tried it myself, now encountering #767 (test failure) on Node.js 12.

P.S. This still looks like a workaround solution, would love to see a more permanent solution on rewire.

@breautek
Copy link
Contributor Author

breautek commented Jul 12, 2019

I tried it myself, now encountering #767 (test failure) on Node.js 12.

At least we can see why it is failing now! hahah

P.S. This still looks like a workaround solution, would love to see a more permanent solution on rewire.

I agree, but I think that will be a waiting game for the rewire people. The workaround should suffice for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants