Details

Reviewers

Habib Gahbiche (zazizizou)
Jacques Lucke (JacquesLucke)

Commits

rB4a02b9ffeb01: Regression Testing: Running tests based on blend files

Summary

Runs tests based on blend files with minimum python interaction.
Developed as part of GSoC 2021 - Regression Testing of Geometry Nodes.
Earlier, tests were built from scratch by adding a modifier/operation
from the Python API.
Now, tests can also be created inside blender and are compared using
Python script.

Features: Automatically adding expected object if it doesn't exist.
This patch adds tests for the following Geometry Nodes category:

Curves
Geometry
Mesh
Points

The implemented UML diagram for refactoring of mesh test framework.

Technical Changes:
SpecMeshTest: It adds the modifier/operation based on the Spec provided.
BlendFileTest: It applies already existing modifier/operation from the blend file.

Test folders hierarchy with tests. This folder should be extracted to lib\tests\modeling

geometry_nodes.zip4 MBDownload

Note: The geometry_nodes folder might lie under another geometry_nodes folder while extracting, please double check. Use the inner-most one.
The hierarchy should be:
-lib\tests\modeling\geometry_nodes\mesh
-lib\tests\modeling\geometry_nodes\points
and so on.

From ctest the tests should be run as ctest -R geo_node -C [Configuration] on Windows.
Each single test can be run with its entire name e..g ctest -R geo_node_geometry_join_geometry.(just an example). Run ctest -N -R geo_node to see all tests.
From blender, the tests can be run blender -b path\to\blend\file --python path\to\geo_node_test.py

Diff Detail

Repository

rB Blender

Branch

soc-2021-geometry-nodes-regression-test (branched from master)

Build Status

Buildable 15244
Build 15244: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Harbormaster completed remote builds in B15727: Diff 39356.Jul 9 2021, 2:37 PM

Himanshi Kalra (calra) marked 2 inline comments as done.Jul 9 2021, 2:39 PM

Himanshi Kalra (calra) added inline comments.

tests/python/modules/mesh_test.py
194 ↗	(On Diff #39346)	Objects are being initialized in the constructor. Aim is if the expected object doesn't exist create it, else initialize the existing. I don't understand why would we want to recreate them?

Renaming: From MeshTest to SpecMeshTest curve_to_mesh tests

Harbormaster completed remote builds in B15728: Diff 39357.Jul 9 2021, 2:47 PM

Overall architecture looks good to me and tests are passing. Revision description seems outdated though. Could you update it to reflect current state? Isn't better to put tests under lib/tests/modelling?

tests/python/CMakeLists.txt
751	Tests will (silently) not run if they don't exist, right? I suggest to check if the directory exists to avoid skipping tests unintentionally
tests/python/modules/mesh_test.py
211 ↗	(On Diff #39357)	Saving here seems unnecessary if you will save the file again after updating the expected object.
267 ↗	(On Diff #39357)	shouldn't you return `self.run_test()` instead? The test can fail on the second run...
276–278 ↗	(On Diff #39357)	This might be a matter of taste but I think this it too short to be a function. You can just write the 2 print lines. If you want to keep the function, then make sure it's properly documented, check the input properly and make it private. Also, the name should be, `print_failed_test_results()` or something similar. In the previous version `failed_test()` does what need to be done when the test fails, i.e. check for update flag, update expected object, save blend file etc..
280 ↗	(On Diff #39357)	same comment for `failed_test()`
328 ↗	(On Diff #39357)	documentation and input checks are missing. Interface seems odd, it says meshes but you only take 1 object name. I think it's better to make it static and put two meshes as argument
364 ↗	(On Diff #39357)	it's hard to guess what this result contains/means without reading implementation. I suggest you return the result as a `dict`. It's also probably better to have one word for a successful comparison, e.g. `success`, so your `result_codes` will look like `{"Validation": 'Invalid Mesh', "Selection": 'sucess', etc...}` This way, the functions that use the result can be more generic. You can iterate over the keys and if not all their respective values are `success` then you print the whole `dict` directly
383 ↗	(On Diff #39357)	doc string seems outdated
670 ↗	(On Diff #39357)	I thought the plan was to get rid of this class?

This revision now requires changes to proceed.Jul 11 2021, 4:37 PM

Updated based on review by Habib Gahbiche

Harbormaster completed remote builds in B15774: Diff 39451.Jul 13 2021, 10:07 AM

Himanshi Kalra (calra) added inline comments.Jul 13 2021, 10:08 AM

tests/python/CMakeLists.txt
751	No. `ctest` will fail with "Not run" if they don't and with "Ran 0 tests" etc if there aren't any. I don't understand the edge case you mention. We iterate over each directory set(geo_node_tests geometry mesh points ) and run tests inside them. They are named category wise. If there is no directory there are no tests.
tests/python/modules/mesh_test.py
211 ↗	(On Diff #39357)	In the case, when `expected_object` added is similar to `test_object` and test doesn't fail. That means, it passes in the first run. Hence it is necessary to save the file with the `expected_object` here.
328 ↗	(On Diff #39357)	Could you elaborate on input checks? expected_object if not present will be caught in the constructor and evaluated_object is by created in the framework itself.
383 ↗	(On Diff #39357)	How so? I intentionally skipped the documentation for the first few arguments, because they are covered in MeshTest already. EDIT: I updated it to include all parameters.
670 ↗	(On Diff #39357)	I am not aware of that. `RunTest` was not part of the UML diagram coz it was not being refactored, hence I skipped that.

Himanshi Kalra (calra) marked 2 inline comments as done and an inline comment as not done.Jul 13 2021, 8:40 PM

Isn't better to put tests under lib/tests/modelling?

Since geometry nodes folder has sub-directories, maybe we can keep them directly under tests and not tests\modeling.

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Jul 13 2021, 8:45 PM

One of the test files has a typo in the name (ponit_distribute.blend).

I couldn't make the geo_node_geometry_test_bounding_box_suzanne test fail (by changing the code). Not sure if I'm doing something wrong. It correctly says Vertex Coordinate Mismatch but the test still succeeds.

114: Switching to fully guarded memory allocator.
114: Blender 3.0.0 Alpha
114: Read blend: /home/jacques/blender-git/lib/tests/geometry_nodes/geometry/bounding_box_suzanne.blend
114: 
114: FAILED bounding_box_suzanne test with the following: 
114: Results:
114: Mesh Comparison : Vertex Coordinate Mismatch
114: Selection Comparison : Same
114: Mesh Validation : Valid
114: 
114: 
114: Blender quit

This revision now requires changes to proceed.Jul 15 2021, 1:07 PM

Himanshi Kalra (calra) planned changes to this revision.Jul 15 2021, 6:38 PM

Generally the patch looks fine to me. The problem with passing tests should obviously get resolved first.

The flag self.verbose is more or less ignored in the new code. I think it's generally fine, because passing --verbose to ctest makes BLENDER_TEST_VERBOSE redundant. But can you make it consistent? Either use self.verbose before printing (uncritical messages) or remove it altogether.

In D11611#306704, @Himanshi Kalra (calra) wrote:

Since geometry nodes folder has sub-directories, maybe we can keep them directly under tests and not tests\modeling.

Having subdirectories doesn't really seem like a good reason to me. So putting it under tests/modeling because geometry nodes is part of modeling sounds better.

tests/python/CMakeLists.txt
751	so it's fine if there are no tests inside a category? If yes then it's ok
tests/python/modules/mesh_test.py
327 ↗	(On Diff #39451)	What's the reason behind exposing evaluated object to the public interface? I think evaluated object is not much different than test and expected objects, so you can store it in `self.evaluated_object` and hide it from outside `MeshTest`

Polishing script: WIP
Polishing script, checking for verbose use cases
Hide evaluated object from MeshTest
Clean up

Harbormaster completed remote builds in B15876: Diff 39647.Jul 16 2021, 2:12 PM

Updated the path for geometry_nodes folder

Harbormaster completed remote builds in B15877: Diff 39650.Jul 16 2021, 2:29 PM

Took care of pseudo-passing tests by raising an Exception.

In D11611#307595, @Habib Gahbiche (zazizizou) wrote:

But can you make it consistent? Either use self.verbose before printing (uncritical messages) or remove it altogether.

I went with printing things which are important, I didn't set a high bar for "critical", indeed the self.verbose has been more extensively used in the old code. The only debatable "print" statement I found is Creating expected object.. one could use self.verbose flag with it, I decided not to because, it might get overlooked that a new object is being created, say due to a spelling error or similar, hence I went in favor of printing this statement. Rest of the print statements are important, IMHO.

tests/python/CMakeLists.txt
751	Yes, I checked it by created an empty folder and then ran tests. I would suggest not having empty folders though.

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Jul 16 2021, 2:34 PM

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Jul 16 2021, 2:55 PM

Works well for me now and the code looks fine as well.

In D11611#307872, @Himanshi Kalra (calra) wrote:

I didn't set a high bar for "critical", indeed the self.verbose

Messages related to internal functionality like "cleaning up..", "creating .. object.." should only get printed when self.verbose is set. Messages related to tests failing or passing should always get printed.

The patch looks fine to me :) Accepting with the expectation that comments will be addressed before landing (I hope I'm doing this correctly, see Campbell's note).

tests/python/geo_node_test.py
34	phabricator doesn't like your file...
tests/python/modules/mesh_test.py
335 ↗	(On Diff #39650)	doc string is missing
432 ↗	(On Diff #39650)	remove?
704 ↗	(On Diff #39650)	Documentation and input checks are missing. You can also only document the base class und then copy it here using something like `apply_operations.__doc__ = MeshTest.apply_operations.__doc__` Also, all other constructors take the name of objects as `str` and not the object itself, so it's probably better to do the same here for consistency reasons.
815–818 ↗	(On Diff #39647)	you can simplify with: `test.apply_modifier = self.apply_modifier` (same for below)

This revision is now accepted and ready to land.Jul 19 2021, 6:10 PM

Addressed comments by Habib
Merge branch 'master' into soc-2021-geometry-nodes-regression-test

Harbormaster completed remote builds in B15913: Diff 39728.Jul 20 2021, 12:00 PM

jim man (jimman2003) awarded a token.Jul 20 2021, 12:29 PM

Merge branch 'master' into soc-2021-geometry-nodes-regression-test
Minor Fix: create_evaluated_object method changed return type fix

Harbormaster completed remote builds in B15939: Diff 39781.Jul 21 2021, 5:19 PM

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Jul 21 2021, 6:11 PM

Add curve tests

Harbormaster completed remote builds in B15941: Diff 39783.Jul 21 2021, 6:12 PM

Himanshi Kalra (calra) retitled this revision from Script for running Geometry Nodes tests from blend files to GSoC 2021: Refactoring of Mesh Test Framework.Jul 23 2021, 8:58 AM

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)

Notes on commit message:
The title is a bit misleading. Your patch falls under "New features and improvements" of commit types. You should at least mention the new feature which is regression testing based on blend files.

Notes on test cases:

Test geo_node_geometry_test_bounding_box_suzanne is failing for me. Requesting changes because I think we shouldn't land this patch with failing tests. You can simply remove it and investigate later.
Some files are really large:
- For example, curve_subdivide.blend is over 10 MB large and each mesh has over 617k face corners. This is excessive for a unit test I think. You should be able to cover all corner cases with a few hundred vertices.
- Same for point_instance.blend
curves folder has .blend1 files which should not be committed.

tests/python/CMakeLists.txt
751	This doesn't work for me. If there are no tests for geometry nodes I get no error message. If for some reason folder names change (by a cleanup commit) then tests won't run and you won't notice anything, which is bad for regression testing.... The downside is that throwing an error at cmake level will force you to have test files locally just to build blender. For me this wouldn't be a problem but maybe Jacques sees it differently...

This revision now requires changes to proceed.Jul 24 2021, 5:50 PM

CMake: Add directory exists check
Merge branch 'master' into soc-2021-geometry-nodes-regression-test

Harbormaster completed remote builds in B16031: Diff 39955.Jul 26 2021, 2:36 PM

Himanshi Kalra (calra) retitled this revision from GSoC 2021: Refactoring of Mesh Test Framework to Regression Testing: Running tests based on blend files.Jul 26 2021, 2:56 PM

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)

Updating cmake error messsage

Harbormaster completed remote builds in B16036: Diff 39966.Jul 26 2021, 6:02 PM

Following tests are failing for me

geo_node_curves_test_curve_endpoints (Failed)
geo_node_curves_test_curve_length (Failed)
geo_node_curves_test_curve_resample (Failed)
geo_node_curves_test_curve_reverse (Failed)
geo_node_curves_test_curve_reverse_attributes (Failed)
geo_node_curves_test_curve_subdivide (Failed)
geo_node_curves_test_curve_to_mesh (Failed)
geo_node_curves_test_curve_to_points_spiral (Failed)
geo_node_curves_test_curve_trim (Failed)
geo_node_mesh_test_subdivide (Failed)

most of them with: Mesh Comparison : Vertex Coordinate Mismatch

Are they all passing for you?

Yes they are are all passing for me from this evening, master at 71d7505487f17ad79eac8a5ac0e

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Jul 27 2021, 2:26 PM

Curve tests were failing due to rB6ac378e685f357e969abcdd327f2ade95

Subdivide test was failing due to different build configuration, Lite doesn't have OpenSubDiv enabled.

The failed tests as reported above have been updated.

Tests are passing again.

This revision is now accepted and ready to land.Jul 27 2021, 3:08 PM

Himanshi Kalra (calra) mentioned this in rBL62661: Adding test files for Geometry Node Tests includes- points, mesh, geometry and….Jul 27 2021, 5:04 PM

Closed by commit rB4a02b9ffeb01: Regression Testing: Running tests based on blend files (authored by Himanshi Kalra (calra)). · Explain WhyJul 27 2021, 5:36 PM

This revision was automatically updated to reflect the committed changes.