cordova-ios
cordova-ios copied to clipboard
[rfc ...] test: use verbose spec reporter
Codecov Report
Merging #643 into master will not change coverage. The diff coverage is
n/a.
@@ 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 dataPowered by Codecov. Last update cfc8177...49cf18b. Read the comment docs.
y?
- easier to see the progress when running
npm teston local workstation - easier to understand when there is a test failure
I am definitely in favour of displaying stacktraces on failed tests.
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.
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.
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 testshould donpm run e2e-testsbefore the slowernpm run objc-tests, which is already done on Travis CInpm run objc-testsseems 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.
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.
Closing as stale and conflicts. Also, in a later release we will attempt to replace Jasmine so this package wont be needed.