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

error thrown prior to QUnit.start causes tests to hang. #8409

Open
stefanpenner opened this issue Feb 6, 2019 · 7 comments
Open

error thrown prior to QUnit.start causes tests to hang. #8409

stefanpenner opened this issue Feb 6, 2019 · 7 comments

Comments

@stefanpenner
Copy link
Contributor

stefanpenner commented Feb 6, 2019

While debugging a "hung CI" build with @chadhietala we noticed that if an error was thrown prior to QUnit.start being invoked, our tests hang forever.

// app.js
throw new Error('this will cause ember test to hang');

Example repo: https://github.com/stefanpenner/__test-chads-problem

@rwjblue
Copy link
Member

rwjblue commented Feb 6, 2019

@stefanpenner - Can you double check that demo repo? I just did:

git clone stefanpenner/__test-chads-problem
cd __test-chads-problem
yarn
yarn test

And all tests pass:

ok 1 Chrome 72.0 - [1 ms] - ESLint | app: app.js
ok 2 Chrome 72.0 - [0 ms] - ESLint | app: resolver.js
ok 3 Chrome 72.0 - [1 ms] - ESLint | app: router.js
ok 4 Chrome 72.0 - [0 ms] - TemplateLint: test-chads-problem/templates/application.hbs
ok 5 Chrome 72.0 - [0 ms] - ESLint | tests: test-helper.js
ok 6 Chrome 72.0 - [0 ms] - ember-qunit: Ember.onerror validation: Ember.onerror is functioning properly

1..6
# tests 6
# pass  6
# skip  0
# fail  0

# ok
   Done in 15.16s.

Maybe you forgot to push a commit with the error inducing code?

@stefanpenner
Copy link
Contributor Author

@rwjblue ya, i likely didn't push yet when you tried it. Try again?

gh stefanpenner/__test-chads-problem                  (29m 55s 689ms)
Cloning into '/Users/spenner/src/stefanpenner/__test-chads-problem'...
remote: Enumerating objects: 38, done.
remote: Counting objects: 100% (38/38), done.
remote: Compressing objects: 100% (28/28), done.
remote: Total 38 (delta 4), reused 38 (delta 4), pack-reused 0
Receiving objects: 100% (38/38), 125.72 KiB | 731.00 KiB/s, done.
Resolving deltas: 100% (4/4), done.
 s/s/__test-chads-problem ╍ yarn && ember test                                                             (1s 594ms)
yarn install v1.13.0
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
[5/5] 🔨  Building fresh packages...
✨  Done in 8.12s.
Environment: test

/Users/spenner/src/stefanpenner/__test-chads-problem/app/app.js
  14:1  error  Unreachable code  no-unreachable

✖ 1 problem (1 error, 0 warnings)

Circular dependency: ../../../../../var/folders/4r/whc65vwj1xggvvky3yy1cp9m000mw4/T/broccoli-97161qFAfdyqM5FN6/cache-082-rollup/build/-private/system/model/model.js -> ../../../../../var/folders/4r/whc65vwj1xggvvky3yy1cp9m000mw4/T/broccoli-97161qFAfdyqM5FN6/cache-082-rollup/build/-private/system/model/internal-model.js -> ../../../../../var/folders/4r/whc65vwj1xggvvky3yy1cp9m000mw4/T/broccoli-97161qFAfdyqM5FN6/cache-082-rollup/build/-private/system/references.js -> ../../../../../var/folders/4r/whc65vwj1xggvvky3yy1cp9m000mw4/T/broccoli-97161qFAfdyqM5FN6/cache-082-rollup/build/-private/system/references/belongs-to.js -> ../../../../../var/folders/4r/whc65vwj1xggvvky3yy1cp9m000mw4/T/broccoli-97161qFAfdyqM5FN6/cache-082-rollup/build/-private/system/model/model.js
cleaning up...
Built project successfully. Stored in "/Users/spenner/src/stefanpenner/__test-chads-problem/tmp/class-tests_dist-6QZFbSUG.tmp".
not ok 1 Chrome 72.0 - [undefined ms] - Global error: Uncaught Error: FAIL at http://localhost:7357/assets/test-chads-problem.js, line 18
    ---
        Log: |
            { type: 'error',
              testContext: {},
              text:
               'Uncaught Error: FAIL at http://localhost:7357/assets/test-chads-problem.js, line 18\n' }
    ...
not ok 2 Chrome 72.0 - [undefined ms] - Global error: Uncaught Error: Assertion Failed: The tests file was not loaded. Make sure your tests index.html includes "assets/tests.js". at http://localhost:7357/assets/vendor.js, line 40278
    ---
        Log: |
            { type: 'error',
              testContext: { state: 'complete' },
              text:
               'Uncaught Error: Assertion Failed: The tests file was not loaded. Make sure your tests index.html includes "assets/tests.js". at http://localhost:7357/assets/vendor.js, line 40278\n' }
    ...

@stefanpenner
Copy link
Contributor Author

stefanpenner commented Feb 6, 2019

// throw here, totally silent error (expected for now);
<script src="/testem.js" integrity=""></script> 
// throw here, all tests are run, but the failure is included

With todays approach global errors after testem.js is loaded are included in the test output, but they do not "fast fail" the test run. Unfortunately there is no way to really tell if these global errors are fatal or not. Currently we assume they are not fatal, and assume the test run will complete. Often this is fine, unfortunately sometimes this will simply cause a test to hang.

Global failures while QUnit (likely also mocha) is in control should be simple handled be QUnit and Mocha.

The issue really is, if an error occurs before QUnit/Mocha take over, we are left in a bad state. Because of this we may want to consider that global errors from the point testem.js is loaded, until 'QUnit.start` begins as fatal.

@stefanpenner
Copy link
Contributor Author

It is also worth noting, this doesn't appear to be regression.

@stefanpenner
Copy link
Contributor Author

@rwjblue et.al Thoughts?

@rwjblue
Copy link
Member

rwjblue commented Feb 8, 2019

I think it would be fine to consider global failures before test framework detection is ran to be fatal...

@void-mAlex
Copy link

We've just run into this exact scenario, causing multiple CI runs to hang due to bad import paths in tests.
the issue was almost immediately fixed and pushed again, but by that time the CI had already started and blocked the pipeline so the build fixing the issue never got started until someone asked why the builds are taking forever.

and there is work to do get people always checking locally before pushing and we'll be reminding developers of that, but it would be greatly appreciated if that didn't just take the CI node out of action.

looking for my issue in the list this one feels like it might be the same thing #8247

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

No branches or pull requests

3 participants