Page MenuHome

Tests: add transform tests
Changes PlannedPublic

Authored by Germano Cavalcante (mano-wii) on Apr 22 2020, 5:31 AM.

Details

Summary

This patch proposes to add basic tests to transform operators.

File to test:

How to test:

  • enable the cmake option WITH_TRANSFORM_TESTS
  • move the transform_regression.blend file to ../lib/tests/modeling/
  • run the command for testing ctest -C Release -R transform

or

  • open the transform_regression.blend file in ../lib/tests/modeling/
  • change the directory in the script to match your blender source file
  • run the script on the file

Diff Detail

Repository
rB Blender
Branch
master
Build Status
Buildable 7669
Build 7669: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Apr 22 2020, 5:31 AM
Germano Cavalcante (mano-wii) created this revision.
Sergey Sharybin (sergey) requested changes to this revision.Apr 22 2020, 9:46 AM

Some quick pass.

Is there an easy way to update this test after some change which has expected behavior changes was made?

tests/python/modules/mesh_test.py
305

This is very weird to have test-specific code in a generic code.
Better design is to have all transform-related code in the transform test. Otherwise the generic code becomes even mode complicated that the actual code you're testing.

tests/python/transform_operators.py
80

I am sure you can make this more readable about what is the input, what is the expected expression.
Having name-less lists and tuples do not help readability and maintainability.

This revision now requires changes to proceed.Apr 22 2020, 9:46 AM

These tests are OK if we just want to check changes to code give the same result, however with larger refactoring (changing operator arguments for e.g), they wont be so helpful.

I'd personally be more interested in tests that use simulated user input, then validate the results are what we expect.

Then we can also check numeric input for eg, or other interactive features...

This is a different kind of test to what your proposing though.


If this is useful for continued development of the transform system, I think it's OK to have these kinds of tests, however when making bigger changes I worry they easily break.

Germano Cavalcante (mano-wii) planned changes to this revision.Apr 22 2020, 1:04 PM
  • Make specific API for transform testing
  • Make everything a little more inline to be easier to maintain
Germano Cavalcante (mano-wii) planned changes to this revision.Apr 23 2020, 3:18 AM

Working on "simulated user input"...

Germano Cavalcante (mano-wii) marked an inline comment as done.
  • Fix test error
  • check if index is not None
  • Attempt to add test with --enable-event-simulate.
  • Add test with event simulation
  • Fix tests in background mode

I struggled with the fact that modal operators are not called in background mode.
The only solution I found was not to use the --background argument.
This looks like a hackistic solution, being the only test that opens a blender window and immediately closes.
It is not easy to get around this problem, even changing the Blender creator code :(

Germano Cavalcante (mano-wii) planned changes to this revision.Apr 29 2020, 12:42 PM

I'll just add a reminder in here that we should add tests for local bone rotation in weight paint mode as well: T85471

  • Rebase on manter
  • Remove parameters that don't work in the background (--enable-event-simulate and --window-geometry)
  • Add test for local bone rotation in weight paint mode
Sybren A. Stüvel (sybren) requested changes to this revision.EditedMar 1 2021, 1:22 PM

I'm replacing @Sergey Sharybin (sergey) by his request. There are a few things that I find a bit strange.

  • open the transform_regression.blend file in ../lib/tests/modeling/
  • run the script on the file

This is the script in question:

import sys
import os

blender_src_dir = os.path.join(__file__[:__file__.find("lib")], "blender")
tests_python_dir = os.path.join(blender_src_dir, "tests", "python")
sys.path.append(tests_python_dir)

def set_args(run_all_tests=True, run_test=0):
    if not "--keep-open" in sys.argv:
        sys.argv.append("--keep-open")

    if run_all_tests:
        if "--run-tests" in sys.argv:
            i = sys.argv.index("--run-tests")
            sys.argv.pop(i)
            sys.argv.pop(i)

        if not "--run-all-tests" in sys.argv:
            sys.argv.append("--run-all-tests")
    else:
        if "--run-all-tests" in sys.argv:
            sys.argv.remove("--run-all-tests")
        if not "--run-test" in sys.argv:
            sys.argv.append("--run-test")
            sys.argv.append("0")

    if "--run-test" in sys.argv:
        i = sys.argv.index("--run-test")
        sys.argv[i + 1] = str(run_test)

def main():
    import imp
    import transform_operators

    set_args(run_all_tests=True, run_test=9)

    imp.reload(transform_operators)
    print('-----------------')
    os.environ["BLENDER_VERBOSE"] = "1"
    transform_operators.main()

if __name__ == "__main__":
    main()
  • The code has no comments that explains what happens, or what the file is even for.
  • There is no comment about the import transform_operators, and which module/file it expects to be importing.
  • The expression os.path.join(__file__[:__file__.find("lib")], "blender") is hard to read and very fragile (it breaks for a user with files in /home/libby/). I would suggest using pathlib instead of os.path.
  • The imp module has been deprecated since Python 3.4 and should definitely not be used in new code.
  • There is just one single call to set_args(), so I don't see the need to even have defaults for the parameters. Use type declarations instead, if you want to clarify their type (👍 from me there).
  • There can apparently be conflicting CLI arguments in sys.argv, which are silently replaced. This is always a bad situation, especially in a testing framework. Conflicting arguments should cause an error situation. If this error situation cannot be fixed at the root, at least there should be an explanation what exactly is the problem, why it cannot be resolved, and why this is silently corrected in this particular spot.

The transform_test.py has no documentation either. A class like TestCompare should really have some explaination about what it represents, and how it's intended to be used. There is also no need to use __slots__; they're nice to reduce memory overhead, but that's only relevant when lots of instances will be created. On the other hand, they do add redundant code (repetition of each attribute) that needs to be reviewed & maintained.

In transform_test.py there are no type declarations. This makes it harder than necessary to understand what is happening. For example, self.on_error = None gives no information beyond that the attribute exists.

Why is transform_operators.py not using Python's unittest module?

When it comes to writing clean code, there are also a bunch of improvements to make, but I think it's more important to get the fundamentals right first.

To drive the point home about a lack of documentation: right now it's not even clear whether transform_test.py is about transforming tests from one form into another, or whether they're testing the transform system.

tests/python/CMakeLists.txt
183–185

What is the reader supposed to do with this information? Why is it relevant here?

tests/python/modules/transform_test.py
22 ↗(On Diff #34500)

The function tries to find a 3D View. This should be clear from the name, and if that's not possible, from a docstring.

26 ↗(On Diff #34500)

Never, ever use a plain raise. Choose the appropriate exception type, and pass it an explanation of what's going wrong.

tests/python/transform_operators.py
26–29

I've never seen this before, and I feel it's also a bit clunky. Most test frameworks use the name of the test, and not its index.

251–264

Why isn't this using argparse instead?

266

What does this do?

This revision now requires changes to proceed.Mar 1 2021, 1:22 PM

In general, don't put test Python scripts in .blend files or the lib/tests folder, that makes them harder to update and refactor.

Also, you can't assume the Blender source folder is named blender. I have multiple git worktrees with different names for example.

Best to avoid reaching from the lib/tests folder back into the source folder altogether and move the script into the source folder.

Germano Cavalcante (mano-wii) marked 5 inline comments as done.
  • Cleanup and documentation
  • Use argparse module

Why is transform_operators.py not using Python's unittest module?

I used another file as a reference (it also doesn't use the unittest module).
I confess that I don't remember much about how to use this module, so it will take me a while to implement it.


Note:
The script in the file is not supposed to be used by Blender.
It is a utility for the developer to use.
So the directory pointed to in tests_python_dir is not really that problematic.

I'm going to modify the script in the file, adding comments and removing deprecated functions.
This script is something that I intend to use a lot, to add tests and fine adjustments.

tests/python/transform_operators.py
26–29

This index is for internal use. The test name is "transform".

This is the same as seen in modifier.py which has an API similar.
Basically transform_operators.py is a modified copy of modifier.py and transform_test.py is a modified copy of mesh_test.py.

266

Removed because it was not meant to be set here.
This environment variable is used in some blender test files.

I will second the idea that it should be OK to put a script in the .blend file to aid the developer. E.g., when I have to debug why a particular bevel test is now failing, I find it easier to do from a script in the file. Not a script to replace the actual test, which remains the authoritative text to specify and execute the test, but as a debugging aid.

Sybren A. Stüvel (sybren) requested changes to this revision.Mar 2 2021, 3:20 PM

I used another file as a reference (it also doesn't use the unittest module).
I confess that I don't remember much about how to use this module, so it will take me a while to implement it.

This is my personal opinion, but I think we should be using built-in functionality of Python unless there is a good reason to deviate.

The script in the file is not supposed to be used by Blender.
It is a utility for the developer to use.

That's only relevant when it's actually documented as such.

So the directory pointed to in tests_python_dir is not really that problematic.

Again, that's only the case if it's easy to read and understand.

The _TestCompare.run function has quite a high cognitive complexity. I think it's possible to greatly simplify the function by spliting some code off into smaller functions. That would leave a run() function like this:

def run(self, index, verbose=True):
    if verbose:
        print()
        print("Test comparison {}: {}".format(index, self.description))

    compare_success = True
    compare_result = ""

    if self.compare_matrix:
        compare_success, compare_result = self._compare_matrix()
    if compare_success and self.compare_data:
        compare_success, compare_result = self._compare_data()

    if not compare_success:
        print("Test comparison result: {}".format(compare_result))
        print("Resulting object '{}' did not match expected object '{}' from file {}".
              format(self.obj_a.name, self.obj_b.name, bpy.data.filepath))
        return False

    if verbose:
        print("Success!")
    return True

Something similar can be done for _dispatch_test_events_func(), as that's also a much too complex function.

I will second the idea that it should be OK to put a script in the .blend file to aid the developer.

I agree, but only if it's documented as such.

tests/python/modules/transform_test.py
44 ↗(On Diff #34608)

Since this class is contained in a test-specific file, and it's not a test itself, there is not really a need to have "Test" in the name. MeshCompare or ObjectCompare or something along those lines would be better.

47 ↗(On Diff #34608)

No need to repeat what's already in the __init__(); these kind of comments tend to rot quickly.

I think it's more important to describe what compare_data actually means.

64–73 ↗(On Diff #34608)

Having break/else/continue/break on consecutive lines makes it really hard to follow what's going on. Move this code into its own function, then you can replace the first break with a return and all the other keywords can go.

65 ↗(On Diff #34608)

Don't use single-letter names.

66 ↗(On Diff #34608)

Don't change the type associated with a name.

What's wrong with a loop like for row in mat_diff: for value_diff in row:?

75–79 ↗(On Diff #34608)

This should also be its own function.

78 ↗(On Diff #34608)

The fact that only mesh objects are supported, and that self.compare_data actually means self.compare_meshes should be documented.

106 ↗(On Diff #34608)

No need to do a late import here.

142 ↗(On Diff #34608)

If I read the code correctly, ret can get two values: None and 0.0. These are not exactly meaningful values, and the handling of ret is not None is so far away from ret = 0.0 that it makes it hard to understand what exactly is going on.

187–190 ↗(On Diff #34608)

What is this mapping for?

218 ↗(On Diff #34608)

Parentheses aren't necessary here. Same for other if-statements.

26 ↗(On Diff #34500)

Of course this comment also applies to all the other plain raise invocations.

tests/python/transform_operators.py
26–29

If there is the need to manually add comments to a list to indicate the index of each item, that's an indication of a bad design.

There is also a comment that explains how to use that index to run a single test, so it's not for internal use either.

modifiers.py identifies tests by name, and not by index, so I don't really see your point.

48

What are "edit mode a" and "b"? What is being tested here? The way the test code is written, it's hard to see the difference between the "a" and "b" functions.

This revision now requires changes to proceed.Mar 2 2021, 3:20 PM
Germano Cavalcante (mano-wii) marked an inline comment as done.Mar 2 2021, 4:47 PM
Germano Cavalcante (mano-wii) added inline comments.
tests/python/modules/transform_test.py
142 ↗(On Diff #34608)

This method is the callback used in timers (I will move it closer, improve description and change the name to make it clear).

When a timers callback returns None, it is unregistered.

If the return is a float, the float value represents the minimum time for the test to be run again.

The fact that the tests need to run on a timers callback makes it difficult to use with unittest module.

Unfortunately this is the only way I have found to be able to use --event-simulate in tests.

(This is also done in other test files).

187–190 ↗(On Diff #34608)

Blender should use a consistent enum for "mode", no matter if it is the mode in the context or the mode of the object.
This is a utility to use with bpy.ops.object.mode_set

I will split things.

tests/python/transform_operators.py
26–29

I may be seeing things, but I'm sure it used to be index.
I don't even know what the names of the modifier tests are.
In that case it may be important to add a --help command to see these names.

Germano Cavalcante (mano-wii) marked 13 inline comments as done.
  • Comment on raise invocations
  • Rename _TestCompare to _ObjectCompare
  • Improve TestContext description
  • Split _ObjectCompare.run method
  • Rename compare_data to compare_meshes
  • Improve timer callback description
  • Split timer's callback
  • Rename e remove TestContext API
  • Rename the test functions in transform_operators.py
  • Optionally run the test with the function name
  • Improve transform_operators.py header
  • Split TestContext._dispatch_test_events_timer_fn callback
Germano Cavalcante (mano-wii) marked an inline comment as done.Mar 2 2021, 11:25 PM
Germano Cavalcante (mano-wii) planned changes to this revision.Apr 9 2021, 10:19 PM