Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gracefully exit with code 1 when build failed #160

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

jooohn
Copy link
Contributor

@jooohn jooohn commented Jun 25, 2019

I found some problems with build command.

  1. It does exit with code 0 even when the build failed. I think it should be 1.
  2. When it fails, the error won't be caught and that causes UnhandledPromiseRejectionWarning.

@jooohn
Copy link
Contributor Author

jooohn commented Jun 25, 2019

Hmm, it's working on my local machine, and I tried this from the same container as Circle CI, but it works successfully...

$ yarn test --runInBand --no-cache --coverage
yarn run v1.16.0
$ jest --config ./test/jest.config.json --runInBand --no-cache --coverage

 RUNS  test/tests/tsdx-build.test.js
✓ Creating entry file 2.7 secs
✓ Building modules 1.3 secs
✓ Creating entry file 2.8 secs
✓ Building modules 1.1 secs
✓ Creating entry file 2.8 secs
(typescript) Error: /Users/j-tomioka/src/github.com/jooohn/tsdx/stage-build/src/index.ts(1,14): semantic error TS2322: Type '"123"' is not assignable to type 'number'.
Error: /Users/j-tomioka/src/github.com/jooohn/tsdx/stage-build/src/index.ts(1,14): semantic error TS2322: Type '"123"' is not assignable to type 'number'.
    at error (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup/dist/rollup.js:9365:30)
    at Object.error (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup/dist/rollup.js:15557:24)
    at Object.error (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup/dist/rollup.js:15986:38)
    at RollupContext.error (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:17187:30)
    at /Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:24954:23
    at arrayEach (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:532:11)
    at forEach (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:9360:14)
    at printDiagnostics (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:24927:5)
    at Object.transform (/Users/j-tomioka/src/github.com/jooohn/tsdx/node_modules/rollup-plugin-typescript2/dist/rollup-plugin-typescript2.cjs.js:26754:17)
 PASS  test/tests/tsdx-build.test.js (14.523s)tsdx/node_modules/rollup/dist/rollup.js:15679:25
 PASS  test/tests/utils-safePackageName.test.ts
----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |      100 |      100 |      100 |      100 |                   |
 util.js  |      100 |      100 |      100 |      100 |                   |
----------|----------|----------|----------|----------|-------------------|

Test Suites: 2 passed, 2 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        17.796s
Ran all test suites.
✨  Done in 18.81s.

@Pruxis
Copy link

Pruxis commented Jun 28, 2019

Yes please! This is kind of blocking anyone from building a decent CI pipeline

@jaredpalmer
Copy link
Owner

Can we make the tests pass?

@jooohn
Copy link
Contributor Author

jooohn commented Jul 1, 2019

Hmm, I don't know why it failed earlier, but now passed...!

@Pruxis
Copy link

Pruxis commented Jul 1, 2019

Thanks @jooohn was going to do it this evening, thanks for giving me more free time ;)

@swyxio
Copy link
Collaborator

swyxio commented Jul 1, 2019

@jooohn what did you do to make it pass? i took a quick look and couldnt figure it out

@jooohn
Copy link
Contributor Author

jooohn commented Jul 1, 2019

basically just retried..

@jaredpalmer jaredpalmer merged commit c850b5c into jaredpalmer:master Jul 5, 2019
@agilgur5
Copy link
Collaborator

@allcontributors please add @jooohn for code, tests

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @jooohn! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants