cordova-ios icon indicating copy to clipboard operation
cordova-ios copied to clipboard

[rfc ...] test: use verbose spec reporter

Open brody4hire opened this issue 6 years ago • 8 comments

brody4hire avatar Jul 14 '19 17:07 brody4hire

Codecov Report

Merging #643 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #643   +/-   ##
=======================================
  Coverage   74.36%   74.36%           
=======================================
  Files          13       13           
  Lines        1845     1845           
=======================================
  Hits         1372     1372           
  Misses        473      473           

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 cfc8177...49cf18b. Read the comment docs.

codecov-io avatar Jul 14 '19 18:07 codecov-io

y?

janpio avatar Jul 14 '19 18:07 janpio

  • easier to see the progress when running npm test on local workstation
  • easier to understand when there is a test failure

brody4hire avatar Jul 14 '19 18:07 brody4hire

I am definitely in favour of displaying stacktraces on failed tests.

breautek avatar Jan 04 '20 16:01 breautek

Any thoughts on having the helper.js file like this?

if (process.env.CI) {
    const { SpecReporter } = require('jasmine-spec-reporter');

    const reporter = new SpecReporter({
        suite: {
            displayNumber: true
        },
        spec: {
            displayErrorMessages: true,
            displayStacktrace: true,
            displaySuccessful: false,
            displayFailed: true,
            displayPending: true,
            displayDuration: true
        },
        summary: {
            displayErrorMessages: false,
            displayStacktrace: false,
            displaySuccessful: false,
            displayFailed: false,
            displayPending: false,
            displayDuration: true
        }
    });

    jasmine.getEnv().clearReporters();
    jasmine.getEnv().addReporter(reporter);
}

The idea that I was thinking is we should only display the failed results and do not show any of the successful tests. This will help minimize the printout.

Additionally, I am not entirely sure if we need this type of printout formatting locally. For sure, there is a benefit in the CI testing services.

erisu avatar Jan 06 '20 06:01 erisu

I am definitely in favour of displaying stacktraces on failed tests.

Agreed, but stacktraces for failed tests are displayed by Jasmine's default reporter as well.

The reason I enabled the verbose logging in cordova-lib was that it has some long-running tests and I wanted to see how long they take and provide better feedback about the currently executing test when running those long-running tests.

Another advantage of the verbose output is the ability to locate the origin of unwanted console output during tests.

But in case that everything goes well (test finish quickly, no garbage printed to console) I actually prefer the more terse default output when running the tests locally.

I like the solution proposed by @erisu that enables verbose output when running on CI services. Additionally, I would suggest also checking another environment variable that only controls this behavior (e.g. CDV_VERBOSE_TESTS=1) so that it can be enabled temporarily when needed locally.

However, to have all advantages that I mentioned above, we should not disable output of successful tests in verbose mode.

raphinesse avatar Jan 12 '20 13:01 raphinesse

Thanks to @erisu and others for the feedback. I still fail to see much benefit of reducing the verbose test output in case of running npm test on a workstation such as mine due to the amount of time I have to wait for the tests to finish. I am also not so happy about the idea of little green dots from Jasmine interspersed with the amount of console logging that comes from e2e tests.

I did make some updates to use the verbose reporter in "e2e-tests", which seems to only include a single create.spec.js test. I can think of a few more things we should consider:

  • move the e2e test into its own subdirectory
  • npm test should do npm run e2e-tests before the slower npm run objc-tests, which is already done on Travis CI
  • npm run objc-tests seems to output a bunch of console output that is hard to follow while waiting for test results, would be nice to see this improved someday

I am keeping this PR in WIP status for now, will understand if others want to close it.

brody4hire avatar Feb 16 '20 18:02 brody4hire

I tried moving npm run objc-tests to the last place in npm test, which seems to be consistent with Travis CI, and changing the spec reporter settings as suggested in https://github.com/apache/cordova-ios/pull/643#issuecomment-571022988, definitely less noisy now. I still like the extra output when waiting for test results on my work station, oh well.

brody4hire avatar Feb 17 '20 14:02 brody4hire

Closing as stale and conflicts. Also, in a later release we will attempt to replace Jasmine so this package wont be needed.

erisu avatar Nov 21 '25 04:11 erisu