Reduce log verbosity in Travis CI environment. #1373

yifanyang · 2018-11-09T00:32:54Z

For succeeded cases, the test in Travis CI environment will no longer display mocha framework logs (for node test) or karma framework logs (for browser test); logs generated by test code itself are still captured and displayed.

Local development still has full logs.

The is to mitigate #535. The number of total log lines on Travis will drop by about 2/3 with this pull request.

Mocha and karma will not show logs for succeeded test cases in CI environment. Local development still displays full logs.

Feiyang1 · 2018-11-09T00:38:35Z

config/mocha.browser.opts

@@ -0,0 +1,2 @@
+--timeout 20000


Do we need to include google license as in the file you deleted?

Looks like only js/ts files require a license header? Js configuration files under the 'config' directory do have a license header, while all the others don't. But I'm not sure. Your call.

P.S. If we decide to add a header in this mocha config file, mocha version needs an upgrade to support the comments.

I don't think we need a license header here. None of other config files that are in .json or other extensions contain a google license header.

Feiyang1 · 2018-11-09T00:38:59Z

config/ci.mocha.node.opts

@@ -0,0 +1,2 @@
+--reporter dot


what does the 2 options do? Is it possible to add comments in the configuration file?

I think we can. That will require us to upgrade mocha to the latest version (5.2.0) in all of our packages. We have 5.0.5 currently.

I can create another pull request to bump the mocha version in all of our SDK packages. Does this kind of change require approvals from all the SDK owners? Or is it okay to upgrade as long as all test continue to pass?

yep, it would require approvals from them. Upgrading should be fine.

Created #1375 to upgrade the mocha version. Feel free to add more reviewers who you think should be aware of this change to that pull request.

Added comments in the file to explain the usage of these flags.

Mocha version in master branch is updated to 5.2.0.

yifanyang · 2018-11-26T18:35:12Z

This pull request is to reduce the amount of logs generated during CI test. Another confusion in our log is that in case of an error, lerna will duplicate the same error messages multiple times. This is annoying especially when the log messages contains many lines (e.g. here firestore logs which has thousands of lines got duplicated 4 times). This is tracked as a separate issue in lerna repo here.

Feiyang1 · 2018-11-26T19:51:01Z

Does lerna duplicate it 4 times? I think lerna just duplicates it once per node and once for browser, so it amounts to 4 error messages?

mikelehen · 2018-11-26T20:24:09Z

I think we also have mocha configured to retry tests 3 times:

firebase-js-sdk/config/mocha.base.js

Line 19 in 1baf7d6

retries: 3

. So that could explain why you see 4 failures.

mikelehen

Do we really need 3 different mocha configs (CI, browser, node)? This seems confusing and I'm concerned that we could end up with different behavior locally vs in travis, which seems very undesirable. Can we just have a single config (or browser vs node if necessary).

mikelehen · 2018-11-26T20:29:23Z

packages/database/package.json

@@ -14,7 +14,7 @@
    "dev": "rollup -c -w",
    "test": "run-p test:browser test:node",
    "test:browser": "karma start --single-run",
-    "test:node": "TS_NODE_CACHE=NO nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --compilers ts:ts-node/register/type-check -r src/nodePatches.ts --retries 5 --timeout 5000 --exit",
+    "test:node": "TS_NODE_CACHE=NO nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --compilers ts:ts-node/register/type-check -r src/nodePatches.ts --retries 5 --timeout 5000 --opts ../../config/mocha.node.opts",


Hrm. This sets retries / timeout and then pulls in options that are going to set different retries / timeout (and I don't know which one will win). Same issue exists elsewhere.

FWIW- 3 retries seems excessive to me already. I don't know why we ended up with 5 retries in places...

Flags on the command line will override the corresponding ones in mocha.opts, based on the mocha doc here. And what I'm trying to propose is to put all default configurations in the config file and only override them with command line flags when necessary.

I have no idea of the 5 retries.

Since we created a config file, we should move all the default options into the file. It will be easier for people to read.

Agreed. I don't think we actually want to override the defaults in these cases. The defaults should work for both CI and local development AFAIK.

(to be clear, by "defaults" I mean what we put in the config file)

mikelehen · 2018-11-26T20:32:06Z

config/ci.mocha.node.opts

+
+# Node doesn't exit after mocha test completion. Supplying this flag for force exit.
+# Not applicable in karma + mocha test (browser test).
+--exit


Am I confused or do we no longer set a timeout / retries in CI?

I'm not opposed to playing with changing these, but I'd want to do it in a separate PR since I think it's orthogonal to the logging output (well, mostly).

This is the mocha config for node testing.

I think it makes sense to separate out the change for mocha config file. I though it would be simple and straightforward at first, but it does feel confusing in this PR now.

yifanyang · 2018-11-26T21:38:36Z

Does lerna duplicate it 4 times? I think lerna just duplicates it once per node and once for browser, so it amounts to 4 error messages?

I think we also have mocha configured to retry tests 3 times:
. So that could explain why you see 4 failures.

Not exactly. See this example for better demonstration. The failed test in the example is a case in firebase/testing, for which we only have node test but no browser test:

firebase-js-sdk/packages/testing/package.json

Lines 10 to 15 in 1baf7d6

    
           "scripts": { 
        
             "build": "rollup -c", 
        
             "dev": "rollup -c -w", 
        
             "test": "TS_NODE_CACHE=NO nyc --reporter lcovonly -- mocha 'test/{,!(browser)/**/}*.test.ts' --require ts-node/register/type-check --retries 5 --timeout 5000 --exit", 
        
             "prepare": "npm run build" 
        
           },

The 4 duplicated messages are:

messages printed by mocha;
messages printed by lerna (prefixed with "lerna ERR!");
messages printed by lerna (in the "Error" filed of the error object at the end of the log);
messages printed by lerna (in the "stdout" field of the error object at the end of the log).

yifanyang · 2019-01-10T00:44:08Z

This pr essentially tries to introduce below two changes. It's more clear to track them separately as these two changes are less related to each other. Closing this pr.

Remove duplicated log generated by lerna. Do not print duplicate error messages to stdout. #1468
Consolidate mocha options into config file. Consolidate mocha configurations into config file. #1469

Reduce log verbosity in Travis CI environment.

63f8809

Mocha and karma will not show logs for succeeded test cases in CI environment. Local development still displays full logs.

yifanyang self-assigned this Nov 9, 2018

yifanyang requested a review from Feiyang1 November 9, 2018 00:32

yifanyang requested review from bojeil-google, depoll, gsoltis, hiranya911, mikelehen, rsgowman, schmidt-sebastian, var-const, wilhuff and zxu123 as code owners November 9, 2018 00:32

google-oss-bot added the needs-triage label Nov 9, 2018

Feiyang1 reviewed Nov 9, 2018

View reviewed changes

yifanyang mentioned this pull request Nov 9, 2018

Upgrade mocha version to 5.2.0 in all packages. #1375

Merged

yifanyang added 2 commits November 21, 2018 14:58

Merge branch 'master' into yifany-log-verbosity

fc7e906

Mocha version in master branch is updated to 5.2.0.

Add comments to explain mocha flag usages.

5ab4d4e

yifanyang force-pushed the yifany-log-verbosity branch from fd4714e to 5ab4d4e Compare November 21, 2018 23:56

yifanyang assigned mikelehen and Feiyang1 Nov 26, 2018

mikelehen suggested changes Nov 26, 2018

View reviewed changes

mikelehen removed their assignment Nov 26, 2018

Feiyang1 added api: core and removed needs-triage labels Jan 9, 2019

yifanyang closed this Jan 10, 2019

yifanyang deleted the yifany-log-verbosity branch January 10, 2019 00:44

firebase locked and limited conversation to collaborators Oct 15, 2019

Reduce log verbosity in Travis CI environment. #1373

Reduce log verbosity in Travis CI environment. #1373

Uh oh!

Conversation

yifanyang commented Nov 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifanyang commented Nov 26, 2018

Uh oh!

Feiyang1 commented Nov 26, 2018

Uh oh!

mikelehen commented Nov 26, 2018

Uh oh!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Feiyang1 Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yifanyang commented Nov 26, 2018

Uh oh!

yifanyang commented Jan 10, 2019

Uh oh!

Uh oh!

Feiyang1 Nov 27, 2018 •

edited

Loading