Meteor-Community-Packages / meteor-mocha

A Mocha test driver package for Meteor 1.3+. This package reports server AND client test results in the server console and can be used for running tests on a CI server or locally.
https://packosphere.com/meteortesting/mocha
MIT License
67 stars 40 forks source link

Client crash can cause tests to succeed #59

Closed steventebrinke closed 6 years ago

steventebrinke commented 6 years ago

When the client crashes before executing any tests, meteor-mocha reports success (exit code 0), since no failing tests were performed. This can be reproduced with the following example code:

if (Meteor.isClient) throw new Error('Client only bug');
it('Failing on client', function() {
    assert.isTrue(Meteor.isServer);
});

Of course, this example is contrived, but for example importing files that are only available server side causes the same problem.

guyemerson commented 5 years ago

Running v1.1.2, I am getting this problem. Errors thrown outside of an it statement cause testing to stop but without recording any failures.

SimonSimCity commented 5 years ago

This error might be thrown before we even register the reporter. I have no idea how we could catch these. If you're not talking about them, please show me a minimal use-case and I'll have a look at it.

guyemerson commented 5 years ago

Minimal working example:

describe('my object', function() {
    it('test1', function() {
        throw new Error('inside it')
    })
    throw new Error('outside it')
    it('test2', function() {
        console.log('never gets here')
    })
})

The first error gets caught, is registered as a test failure, and is displayed. The second error does not get caught, and testing stops. The error can be seen in the browser console, but the browser displays that "100%" of tests have run, even though this test and all subsequent tests have not run. It would be great if the display could somewhere indicate that an error was thrown and testing had to stop.

SimonSimCity commented 5 years ago

I cannot confirm that. To me it does show that an error was thrown on the client side. Please show me a project where it fails, and where you use the latest version of both packages meteortesting:mocha-core and meteortesting:mocha.

guyemerson commented 5 years ago

I have just updated to the latest version of both packages, in this project: https://github.com/bigstas/MinimalBears/tree/development

I still have this problem. I am running tests using meteor test --driver-package meteortesting:mocha --port 3100

SimonSimCity commented 5 years ago

@guyemerson you're using an old version of meteortesting:mocha-core. Please follow the steps described here: https://github.com/meteortesting/meteor-mocha/issues/88

guyemerson commented 5 years ago

As I said in the previous post, I upgraded. The current version is v6.1.2.

guyemerson commented 5 years ago

Any other suggestions?

SimonSimCity commented 5 years ago

Today I've tried it again:

  1. Created a new project by running meteor create test
  2. Move into the directory and install the npm package puppeteer (npm install puppeteer)
  3. Install this package from atmosphere: meteor add meteortesting:mocha
  4. Update all atmosphere packages possible: meteor update; meteor update --all-packages
  5. Manipulate the tests-file
  6. Run TEST_BROWSER_DRIVER=puppeteer meteor test --driver-package meteortesting:mocha --raw-logs --once

And it fails - loudly. It also does when removing the meteor.testModule section and running the tests by the auto-include feature. echo $? does also return 1 ...

guyemerson commented 5 years ago

I think there may have been a misunderstanding -- I was referring to the GUI display in the browser, without using TEST_BROWSER_DRIVER, but using TEST_WATCH = 1.

I can confirm that when running tests as you suggest, the error is reported clearly in the console.

When running tests in a normal end-user browser (as I have been doing), the error is reported in the browser console but not in the browser window. When I previously said that the browser displays that "100%" of tests have run, I was referring to the progress wheel that is displayed at the top right of the browser window. It displays "100%" even if the error caused testing to stop early and some tests were not run.

I hope this is now enough information to re-create this bug.

guyemerson commented 5 years ago

In our use case, we can move code to a before() block, where the error can be safely caught and the failure reported in the browser window. I suspect this would also be sufficient for the original poster's suggested use case of importing files.

As you mentioned in a previous comment, "This error might be thrown before we even register the reporter. I have no idea how we could catch these." It may be that there is no straightforward way to catch such errors, and so putting code inside a before() block could be considered best practice.

SimonSimCity commented 5 years ago

This is then part of the reporter "HTML", which in turn is an internal reporter of the mocha project: https://github.com/mochajs/mocha/blob/master/lib/reporters/html.js