cli: implement --failAfterWarnings flag #3712
Conversation
tjenkinson
force-pushed
the
cli-fail-after-warnings
branch
from
6fdc3ef
to
29a9465
Compare
| @@ -0,0 +1,5 @@ | |||
| 'use strict'; | |||
|
|
|||
| require('unknown'); | |||
There was a problem hiding this comment.
don't think this is been checked with error: () => true in this or other tests. will investigate
There was a problem hiding this comment.
Please do. I checked e.g. node-config-not-found and changing the expected output there turned the test red.
There was a problem hiding this comment.
In this case it's because I wasn't returning true to make it continue. In the other tests where _expected.js was being ignored I removed _expected.js because they didn't match, and I'm not sure if the intent was to check the output or not?
There was a problem hiding this comment.
You need to return true in order to continue
Line 42 in 8333387
_expected.js should be compared to stdout, but only if there is no _expected dir, config.test, config.result, config.execute, config.stderr that does not return true or config.error that does not return true.
Codecov Report
@@ Coverage Diff @@
## master #3712 +/- ##
=======================================
Coverage 96.96% 96.97%
=======================================
Files 184 184
Lines 6402 6408 +6
Branches 1854 1855 +1
=======================================
+ Hits 6208 6214 +6
Misses 103 103
Partials 91 91
Continue to review full report at Codecov.
|
|
Nice work. I will try to have a proper look shortly. |
| @@ -49,6 +49,7 @@ Basic options: | |||
| --preserveSymlinks Do not follow symlinks when resolving files | |||
| --shimMissingExports Create shim variables for missing exports | |||
| --silent Don't print warnings | |||
| --failAfterWarnings Exit with an error code if there was a warning during the build | |||
There was a problem hiding this comment.
We also need to extend documentation in the docs folder, just check where and how e.g. --enivronment is documented.
Summary: We’ve recently eliminated all Rollup warnings. This prevents regression. Rollup upgrade needed for `--failAfterWarnings`, added recently: <rollup/rollup#3712> Test Plan: Modify the `onwarn` handler in our `rollup_config.js` to remove any of the early-`return`s, and note that `bazel build //tensorboard` fails. wchargin-branch: rollup-no-warn wchargin-source: 8019e8292a51aa9f74cae2d1acc8b0e5ce47cdb2
Summary: We’ve recently eliminated all Rollup warnings. This prevents regression. Rollup upgrade needed for `--failAfterWarnings`, added recently: <rollup/rollup#3712> Test Plan: Modify the `onwarn` handler in our `rollup_config.js` to remove any of the early-`return`s, and note that `bazel build //tensorboard` fails. wchargin-branch: rollup-no-warn
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers: closes #3021
Description
Adds the
--failAfterWarningsflag which when provided causes the command to error if there were any warnings.TODO
error: () => true