Fix some unit tests not testing properly #12778

OndrejSpanel · 2017-11-29T10:26:49Z

Several unit tests contained mistakes which made them not testing properly what they should, mostly because they were passing wrong arguments to the tested function. They were all passing and are still passing now, the tests were fixed as needed.

I have also added argument checks into the called functions to detect such parameter/argument mismatches.

The case of Frustum.intersectsBox is most confusing to me, I am not sure how the code could be passing before:

box.translate( - 1, - 1, - 1 );

It definitely was not passing when changed to box.translate( new Vector3( - 1, - 1, - 1 ) );, epsilon had to be added.

The case of Euler order seems quite clear.

The Face3 fix is most debatable - the tests were not wrong, but I am convinced they are more robust now.

OndrejSpanel · 2017-11-29T13:56:44Z

Please check #12778, too.

@Mugen87 I have already reworked the commits meanwhile. Do you see any issues still left?

Mugen87 · 2017-11-29T14:06:14Z

src/math/Box3.js

@@ -467,6 +467,9 @@ Object.assign( Box3.prototype, {
 	}(),

 	translate: function ( offset ) {
+		if ( ! offset.isVector3 ) {
+            throw new Error( 'THREE.Box3: .translate() expects a Vector3.' );


Indentations not correct here...

I hope to have cleaned all of them.

BTW. You can use an ESLint plugin in your editor or IDE for better code style. three.js uses an ESLint preset that is configured in the package JSON. Linter plugins like the ESLint plugin for Atom automatically detect this present and show appropriate feedback in the editor.

OK. ESLint plugin installed, this will definitely make things easier for me in the future.

I think code style in this PR is good already (ESList also does not show any issues to me). Or are there still some issues left?

I thinks it's okay now.

Mugen87 · 2017-11-29T14:06:28Z

src/math/Matrix4.js

+        if ( order ) {
+
+            throw new Error( 'THREE.Matrix4: .makeRotationFromEuler() no longer accepts order as the second parameter.' );
+        }


Same here...

Mugen87 · 2017-11-29T14:06:35Z

src/math/Quaternion.js

+        if ( typeof update === 'string' || update instanceof String ) {
+
+            throw new Error( 'THREE.Quaternion: .setFromEuler() no longer accepts order as the second parameter.' );
+		}


And here...

Mugen87 · 2017-11-29T14:06:51Z

test/unit/src/math/Quaternion.tests.js

-			var m3 = new Matrix4().makeRotationFromEuler( angles[ 2 ], "XYZ" );
+			var m1 = new Matrix4().makeRotationFromEuler( changeEulerOrder( angles[ 0 ], "XYZ" ) );
+            var m2 = new Matrix4().makeRotationFromEuler( changeEulerOrder( angles[ 1 ], "XYZ" ) );
+            var m3 = new Matrix4().makeRotationFromEuler( changeEulerOrder( angles[ 2 ], "XYZ" ) );


Mugen87 · 2017-11-29T14:07:20Z

test/unit/src/math/Quaternion.tests.js

@@ -160,6 +160,10 @@ function slerpTestSkeleton( doSlerp, maxError, assert ) {

 }

+function changeEulerOrder(euler, order) {
+	return new Euler( euler.x, euler.y, euler.z, order );
+}


Like this:

function changeEulerOrder( euler, order ) { return new Euler( euler.x, euler.y, euler.z, order ); }

…mEuler, throw on old parameters use.

…ed copy (some fields missing) would pass.

mrdoob · 2017-12-01T11:19:52Z

src/math/Matrix4.js


 		if ( ! ( euler && euler.isEuler ) ) {

 			console.error( 'THREE.Matrix4: .makeRotationFromEuler() now expects a Euler rotation rather than a Vector3 and order.' );

 		}
+		if ( order ) {
+
+			throw new Error( 'THREE.Matrix4: .makeRotationFromEuler() no longer accepts order as the second parameter.' );


Similarly here. I would prefer to add these checks in the tests instead.

They are not needed at all. If you prefer, I can remove them completely, test do not test for them. I just added them thinking that if the test code made this mistake, real client code might make it as well.

I understand, but yeah, I would prefer not to modify src for now.

mrdoob · 2017-12-01T11:54:45Z

src/math/Box3.js

@@ -468,6 +468,12 @@ Object.assign( Box3.prototype, {

 	translate: function ( offset ) {

+		if ( ! offset.isVector3 ) {
+
+			throw new Error( 'THREE.Box3: .translate() expects a Vector3.' );


Same here 😇

Do you mind removing this check?
Please, don't modify files in src/ for test PRs.

mrdoob · 2017-12-01T11:54:59Z

src/math/Matrix4.js

@@ -150,7 +150,7 @@ Object.assign( Matrix4.prototype, {

 	}(),

-	makeRotationFromEuler: function ( euler ) {
+	makeRotationFromEuler: function ( euler, order ) {


order is no longer needed.

Commit ammended - parameter removed.

OndrejSpanel · 2017-12-04T08:34:43Z

As requested - src unmodified again.

I think some static analysis checking for parameter mismatches would be handy, I will be posting more fixes with silly mistakes (tests again), where wrong number of parameters or wrong types are involved. I am not experienced in JS, therefore I have no idea what tooling is available, but I think it is too easy now to make simple mistakes which go undetected.

…2Array instead of InterleavedBuffer.

mrdoob · 2017-12-05T05:06:30Z

Thanks!

OndrejSpanel force-pushed the unitTestFixes branch 2 times, most recently from 30e5ccf to 9641290 Compare November 29, 2017 13:19

Mugen87 mentioned this pull request Nov 29, 2017

Trying to fix geometry tests #12781

Merged

Mugen87 requested changes Nov 29, 2017

View reviewed changes

OndrejSpanel added 3 commits November 29, 2017 15:21

Fix parameters of Quaternion.setFromEuler and Matrix4.makeRotationFro…

76b2ff4

…mEuler, throw on old parameters use.

Fix intersect test parameters to translate.

1ef8062

Testing copy into itself does not seem very robust, wrongly implement…

5a6acb6

…ed copy (some fields missing) would pass.

OndrejSpanel force-pushed the unitTestFixes branch from 9641290 to 5a6acb6 Compare November 29, 2017 14:25

Mugen87 approved these changes Nov 30, 2017

View reviewed changes

mrdoob reviewed Dec 1, 2017

View reviewed changes

Removed parameter validate as requested.

9a0de32

OndrejSpanel force-pushed the unitTestFixes branch from b594ec6 to 9a0de32 Compare December 2, 2017 09:31

Removed parameter validation from Box3.translate

209e05a

OndrejSpanel added 3 commits December 4, 2017 09:43

Fixed "new InterleavedBuffer" parameters: stride was passed to Float3…

16631c7

…2Array instead of InterleavedBuffer.

Fixed "ok" parameters: comma used instead of &&.

6c3d1e9

Fixed: wrong parameters passed to Tetrahedron constructors.

2857b0b

mrdoob merged commit 17452bb into mrdoob:dev Dec 5, 2017

OndrejSpanel deleted the unitTestFixes branch October 11, 2018 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some unit tests not testing properly #12778

Fix some unit tests not testing properly #12778

OndrejSpanel commented Nov 29, 2017

OndrejSpanel commented Nov 29, 2017 •

edited

Loading

Mugen87 Nov 29, 2017

OndrejSpanel Nov 29, 2017

Mugen87 Nov 29, 2017

OndrejSpanel Nov 30, 2017

Mugen87 Nov 30, 2017

Mugen87 Nov 29, 2017

Mugen87 Nov 29, 2017

Mugen87 Nov 29, 2017

Mugen87 Nov 29, 2017

mrdoob Dec 1, 2017

OndrejSpanel Dec 1, 2017 •

edited

Loading

mrdoob Dec 1, 2017

mrdoob Dec 1, 2017

mrdoob Dec 4, 2017 •

edited

Loading

mrdoob Dec 1, 2017

OndrejSpanel Dec 2, 2017

OndrejSpanel commented Dec 4, 2017

mrdoob commented Dec 5, 2017

Fix some unit tests not testing properly #12778

Fix some unit tests not testing properly #12778

Conversation

OndrejSpanel commented Nov 29, 2017

OndrejSpanel commented Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrdoob Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OndrejSpanel commented Dec 4, 2017

mrdoob commented Dec 5, 2017

OndrejSpanel commented Nov 29, 2017 •

edited

Loading

OndrejSpanel Dec 1, 2017 •

edited

Loading

mrdoob Dec 4, 2017 •

edited

Loading