really run all tests and fix broken tests #5123

vincentfretin · 2022-09-24T10:32:17Z

I'm still not sure how karma file pattern works to find the tests but
npm run test:chrome
was running actually only the tests from the tests/components/ directory.
This was the equivalent of
TEST_FILE=components npm run test:chrome
355 tests

With the changes, specifying each directory, it finds all the tests now:
npm run test:chrome
511 tests, 2 skipped

I fixed the broken tests.

vincentfretin · 2022-09-24T11:05:15Z

tests/components/shadow.test.js

        new THREE.MeshBasicMaterial({color: 0xffff00})
      );
      meshWithMaterialArray = new THREE.Mesh(
-        new THREE.Sphere(2),
+        new THREE.SphereGeometry(2),


THREE.Sphere exists but the arguments are center vec3 and radius number in latest threejs... I spend really too much time to find that the cryptic error was because of this lol. THREE.SphereGeometry first arg is radius number.

vincentfretin · 2022-09-24T11:07:11Z

tests/systems/camera.test.js

@@ -147,7 +147,7 @@ suite('camera system', function () {
          assert.ok(cameraEl.getAttribute('camera').active);
          assert.ok(cameraSystem.setActiveCamera.calledOnce);
          assert.equal(cameraSystem.activeCameraEl, cameraEl);
-          cameraSystem.setActiveCamera.reset();
+          cameraSystem.setActiveCamera.resetHistory();


The sinon api changed. I already did a similar change in other files when I did #5091

dmarcos · 2022-09-25T21:43:10Z

tests/karma.conf.js

+  FILES.push('tests/extras/**/*.test.js');
+  FILES.push('tests/shaders/**/*.test.js');
+  FILES.push('tests/systems/**/*.test.js');
+  FILES.push('tests/utils/**/*.test.js');


Problem with this is that if we add a new directory we have to manually update this list. I anticipate frustration :) Any insight why the previous was no longer working?

Note that with karma-browserify before or after the big update of all the test stack in #5091, it also ran only 355 tests. After the switch to karma-webpack, it would only run 16 tests (corresponding to the core directory it seems), so I changed to use glob in https://github.com/aframevr/aframe/pull/5116/files#diff-728e6c3454f7b441786065d02f09e5cefeadfc2c007bf03b44d03f43dfc1a8c5 to run again 355 tests, but that's not actually all the tests. It's been like that for years it seems.
I thought first this had probably something to do with the way karma preprocess test files, and/or the difference how karma-browserify and karma-webpack inject additional files to the list, and the order maybe, but looking at the code I couldn't find really what's going on.
https://github.com/nikku/karma-browserify/blob/17967164c91e4d5644cca26bd8f84e3674434351/lib/bro.js#L84-L121
https://github.com/ryanclark/karma-webpack/blob/f90487b0ca4c342b546e78b4dbad6fc7f50638d8/lib/karma-webpack/preprocessor.js#L17-L53
https://github.com/ryanclark/karma-webpack/blob/f90487b0ca4c342b546e78b4dbad6fc7f50638d8/lib/karma-webpack/framework.js#L4-L32
https://karma-runner.github.io/latest/config/files.html

I did some debugging with only the FILES.push('tests/**/*.test.js'); pattern.
I put some console.log before the return in
https://github.com/karma-runner/karma/blob/f2d0663105eba0b9ea7f281230546282a46015ad/lib/file-list.js#L117-L151

if (p.pattern === '/home/vincentfretin/workspace/aframe/tests/**/*.test.js') { console.log("included", Object.keys(included).length); console.log("lookup", Object.keys(lookup).length); }

When I run I get all the test files
included 93
lookup 109
but only 16 tests are run.

find tests/ -name "*test.js"|wc -l
87 files

I also added a console.log in karma.js where it serves all the scripts, each test.js is bundled with webpack and included in a script tag. I get the 95 files included with a script tag there.

If now I use

glob.sync('tests/**/*.test.js').forEach(function (filename) { FILES.push(filename); });

I get the same 95 files included as scripts in the same order but I get 355 tests. No idea what's going on here...

If I use

FILES.push('tests/components/**/*.test.js'); FILES.push('tests/core/**/*.test.js'); FILES.push('tests/extras/**/*.test.js'); FILES.push('tests/shaders/**/*.test.js'); FILES.push('tests/systems/**/*.test.js'); FILES.push('tests/utils/**/*.test.js');

I have 95 files included, but in a different order, and I get 509 tests.

It's probably an issue with karma-mocha/mocha really, it seems we actually have several asynchronous tests that are not properly written with a done() so they actually fail between tests and come back wrongly green, and if there is an error between tests, mocha stops executing tests, see karma-runner/karma-mocha#227

We can perhaps do this to avoid listing the directories by hand?

glob.sync('tests/*/').forEach(function (dirname) { FILES.push(dirname + '**/*.test.js'); });

Ok. nevermind. It's the order... mmmh. We will have similar issues in the future as we add tests and will have to fiddle with the order.

The tests folders are a one-to-one mapping of the folders in src. I don't think we will be adding a folder anytime soon. But sure to be future proof we can do as you suggested but I'll include ignoring assets, coverage, node folders to avoid warnings:

27 09 2022 09:20:58.769:WARN [filelist]: Pattern "/home/vincentfretin/workspace/aframe/tests/assets/**/*.test.js" does not match any file. 27 09 2022 09:20:58.769:WARN [filelist]: Pattern "/home/vincentfretin/workspace/aframe/tests/coverage/**/*.test.js" does not match any file. 27 09 2022 09:20:58.769:WARN [filelist]: Pattern "/home/vincentfretin/workspace/aframe/tests/node/**/*.test.js" does not match any file.

This is currently working the same:

var excluded_folders = ['assets', 'coverage', 'node']; glob.sync('tests/*/').forEach(function (dirname) { if (excluded_folders.indexOf(dirname) !== -1) return; FILES.push(dirname + '**/*.test.js'); });

The real fix would be to fix the tests so we are not dependent of the order that's for sure, but I couldn't find the bad test.

vincentfretin · 2022-09-26T12:35:04Z

There is actually a lot more tests! But I had to use suite.skip or test.skip for some tests to make all the tests runs for now
with

FILES.push('tests/components/**/*.test.js');
FILES.push('tests/core/**/*.test.js');
FILES.push('tests/extras/**/*.test.js');
FILES.push('tests/shaders/**/*.test.js');
FILES.push('tests/systems/**/*.test.js');
FILES.push('tests/utils/**/*.test.js');

I get
✔ 1031 tests completed
ℹ 100 tests skipped

With

FILES.push('tests/**/*.test.js');

so it's changing the order of test execution, it currently crashes at

    ✔ set tracked-controls-webvr if WebVR API available
  visible
    update
      ✔ treats empty as true
      ✔ can set to visible
    ✖ "before each" hook in "visible"
      ✔ can set to not visible
ERROR: 'An uncaught exception was thrown between tests'
  ✖ "after each" hook for "can set to not visible"

Finished in 16.691 secs / 1.669 secs @ 14:33:11 GMT+0200 (heure d’été d’Europe centrale)

SUMMARY:
✔ 1027 tests completed
ℹ 18 tests skipped
✖ 2 tests failed

FAILED TESTS:
  visible
    ✖ "before each" hook in "visible"
      Chrome 105.0.0.0 (Linux x86_64)
    Error: done() called multiple times in hook <visible "before each" hook in "visible">; in addition, done() received error: Error: Uncaught AssertionError: expected false to be truthy (node_modules/chai/chai.js:250)

  ✖ "after each" hook for "can set to not visible"
    Chrome 105.0.0.0 (Linux x86_64)
  AssertionError: expected false to be truthy
      at build/commons.js:55760:14

vincentfretin · 2022-09-26T13:52:19Z

Error: done() called multiple times in hook <visible "before each" hook in "visible">
means that a previous test didn't call done() because el.hasLoaded was true lol.

vincentfretin · 2022-09-26T15:05:22Z

There is a progress:
✔ 1091 tests completed
ℹ 40 tests skipped

vincentfretin · 2022-09-26T16:31:01Z

✔ 1096 tests completed
ℹ 35 tests skipped

remaining failing tests are one raycaster test and I don't understand why it's failing, and the a-assets test suite that stops running all next tests because of an "ERROR: 'An uncaught promise rejection occurred between tests'" so we probably need to fix the code itself, but I see there is also #5033 pending.

vincentfretin · 2022-09-26T16:49:17Z

✔ 1118 tests completed
ℹ 13 tests skipped

There are still 5 skipped tests of a-assets related to error and timeout and one for raycaster we need to figure out.

vincentfretin · 2022-09-26T18:30:56Z

My last commit fixes the issues I had with a-assets test suite, I may also have fixed the #5032 issue that #5033 was fixing @diarmidmackenzie ? I need to test some real cases for this issue, maybe add the console warning like you did in your PR?

✔ 1122 tests completed
ℹ 9 tests skipped

There is still the raycaster test that bother me.

vincentfretin · 2022-09-27T07:19:40Z

I also have sometimes the error

    button colors
      ✖ has trigger at default color
ERROR: 'An uncaught exception was thrown between tests'
  ✖ "after each" hook for "has trigger at default color"

Finished in 12.644 secs / 5.256 secs @ 09:12:42 GMT+0200 (heure d’été d’Europe centrale)

SUMMARY:
✔ 480 tests completed
ℹ 3 tests skipped
✖ 3 tests failed

FAILED TESTS:
  vive-controls
    model
      ✖ loads
        Chrome 105.0.0.0 (Linux x86_64)
      Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

    button colors
      ✖ has trigger at default color
        Chrome 105.0.0.0 (Linux x86_64)
      Error: Uncaught AssertionError: expected undefined to be truthy (node_modules/chai/chai.js:250)
          at module.exports.Object.create.emit.value (build/commons.js:28216:16)
          at build/commons.js:19453:14
          at Object.onLoad (build/commons.js:66503:9)
          at build/commons.js:59938:168

  ✖ "after each" hook for "has trigger at default color"
    Chrome 105.0.0.0 (Linux x86_64)
  AssertionError: expected undefined to be truthy
      at HTMLElement.<anonymous> (build/commons.js:45612:16)
      at module.exports.Object.create.emit.value (build/commons.js:28216:16)
      at build/commons.js:19453:14
      at Object.onLoad (build/commons.js:66503:9)
      at build/commons.js:59938:168

but from my understanding the test looks ok, model-loaded listener is registered before setting the model.

If I reexecute again I have

✔ 1122 tests completed
ℹ 9 tests skipped

The 2s limit to run the test that load a big obj model may just not be enough in this particular case.

vincentfretin · 2022-09-27T07:42:39Z

I increased the mocha timeout from the default 2s to 3s, hopefully it will fix the random issue with the above test.

…sing but gave an error TypeError: Cannot read properties of undefined (reading 'Component')

…ardown that make the test fail the second time

vincentfretin · 2022-09-27T12:50:53Z

I fixed the raycaster test.

I think this is ready to merge.
For the load scene after a resource 404 or timeout, the behavior didn't change. The fix in #5033 will still be needed.

dmarcos · 2022-09-30T06:43:03Z

Thank you x 1000

really run all tests and fix broken tests

a507cfa

vincentfretin commented Sep 24, 2022

View reviewed changes

vincentfretin mentioned this pull request Sep 24, 2022

Wait for DOMContentLoaded before checking if we have a camera in the DOM #5124

Closed

dmarcos reviewed Sep 25, 2022

View reviewed changes

vincentfretin added 5 commits September 26, 2022 12:55

fix sinon spy

c5dfd55

fix some asynchronous tests

993ad4a

fix camera mock

9ff44b8

use elFactory instead of deprecated entityFactory

ca2df2a

fail test if there is an error between tests

93467da

properly call done() if el.hasLoaded

2e78b49

vincentfretin force-pushed the really-run-all-tests branch from d755317 to 264dc77 Compare September 26, 2022 15:04

vincentfretin added 4 commits September 26, 2022 17:41

fix mobile test that calls screen.orientation.lock

df45581

fix error with requestFullscreen

da99fa2

fix camera test

8cfe27a

skip one raycaster test and all a-assets test suite for now

87e6936

vincentfretin force-pushed the really-run-all-tests branch from 264dc77 to 87e6936 Compare September 26, 2022 16:25

skip only a-assets test related to error and timeout

73f05c6

fix 4 a-assets tests by using Promise.allSettled

35172d6

vincentfretin mentioned this pull request Sep 26, 2022

Possible fix for #5032: Don't hang forever on asset loading error #5033

Merged

add a comment about the global pattern

3875098

vincentfretin added 2 commits September 27, 2022 09:35

use a glob to find the folders instead of hard coding the list

28fdfd7

increase mocha timeout from 2s to 3s

94efb79

vincentfretin added 3 commits September 27, 2022 11:44

avoid some TypeError setting image on null during tests

2c6984f

don't make assumption of init order in those two tests, they were pas…

fe149ab

…sing but gave an error TypeError: Cannot read properties of undefined (reading 'Component')

be sure we don't have a second raycaster-intersected fired in test te…

8fba06f

…ardown that make the test fail the second time

dmarcos merged commit c6ce0f0 into aframevr:master Sep 30, 2022

vincentfretin deleted the really-run-all-tests branch September 30, 2022 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

really run all tests and fix broken tests #5123

really run all tests and fix broken tests #5123

vincentfretin commented Sep 24, 2022

vincentfretin Sep 24, 2022

vincentfretin Sep 24, 2022

dmarcos Sep 25, 2022

vincentfretin Sep 26, 2022

dmarcos Sep 27, 2022 •

edited

Loading

dmarcos Sep 27, 2022

vincentfretin Sep 27, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 27, 2022

vincentfretin commented Sep 27, 2022

vincentfretin commented Sep 27, 2022

dmarcos commented Sep 30, 2022

really run all tests and fix broken tests #5123

really run all tests and fix broken tests #5123

Conversation

vincentfretin commented Sep 24, 2022

vincentfretin Sep 24, 2022

Choose a reason for hiding this comment

vincentfretin Sep 24, 2022

Choose a reason for hiding this comment

dmarcos Sep 25, 2022

Choose a reason for hiding this comment

vincentfretin Sep 26, 2022

Choose a reason for hiding this comment

dmarcos Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

dmarcos Sep 27, 2022

Choose a reason for hiding this comment

vincentfretin Sep 27, 2022

Choose a reason for hiding this comment

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 26, 2022

vincentfretin commented Sep 27, 2022

vincentfretin commented Sep 27, 2022

vincentfretin commented Sep 27, 2022

dmarcos commented Sep 30, 2022

dmarcos Sep 27, 2022 •

edited

Loading