Page MenuHome

Gsoc 2020 Testing frameworks (Soc branch diff against master)
ClosedPublic

Authored by Himanshi Kalra (calra) on Aug 8 2020, 1:56 PM.

Details

Summary

This revision contains the following changes-

  • A unique test name is added to existing tests.
  • Tests for Physics modifiers and remaining Generate and Deform modifiers are added.
  • The existing ModifierSpec is updated with backward compatibility to support Physics Modifiers.
  • Now there is support for frame number and giving nested parameters for attributes.
  • Some Deform modifiers required Object Operators, e.g. "Bind" in Mesh Deform, so a new class was added to support that functionality.
  • A test object generator script which tries to minimize the effort and time spend on creating test objects.
  • A separate class for holding Particles System, they are tested by converting all the particles to mesh and joining it to the mesh they were added.

Note

  • Extract "modifier_test_files" in lib\tests\modeling

  • Extract "physics_test_files" in lib\tests\physics

Diff Detail

Repository
rB Blender
Branch
soc-2020-testing-frameworks
Build Status
Buildable 11795
Build 11795: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Solved merge conflict
  • removed extra str in cloth
  • saving state, not running compare unit test for run-test
  • Merge branch 'master' into soc-2021-testing-frameworks
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Sep 1 2020, 9:41 PM
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Sep 1 2020, 10:11 PM
  • Added comment for not comparing in run-test and message
  • Merge branch 'master' into soc-2020-testing-frameworks
  • Added apply_modifiers flag for run-test and run-all-tests
  • Added a new compare flag do_compare for run-test
tests/python/modules/mesh_test.py
133

doesn't seem right

431

should be renamed _apply_operator_edit_mode()

475

for consistency, you could rename to _apply_operator_object_mode()

582

are these tabs? if so you should replace with spaces

tests/python/physics_cloth.py
33

the line after this comment does exactly the opposite of what the comment says, it does add a modifier.
Or maybe this was meant as an instruction on how to add tests? in that case you should add such instructions in doc strings or in your wiki page

  • Post Review: Minor fixes, renaming, removing confuisng comments
Himanshi Kalra (calra) marked 7 inline comments as done.Sep 6 2020, 11:38 PM

Patch looks good to me.

Note:
Many python tests fail on linux and mac including these ones because of memory leak. This only happens when building with make developer where memory leaks are treated as errors in unit tests (since D8665). I believe this issue is unrelated to this patch, so for me this patch is good enough to be merged in master (and profit from the newly introduced automated tests).

This revision is now accepted and ready to land.Sep 8 2020, 8:06 PM

LGTM too, all affected tests are now passing for me.

@Habib Gahbiche (zazizizou) yes, ASAN has to be disabled currently for tests unfortunately, as it always reports issues with third party libs… Nothing new here, this is indeed totally unrelated to that work.

@Campbell Barton (campbellbarton) may also want to have a look at that code before it lands in master?

tests/python/modules/mesh_test.py
65

Missing a space before int

88

Again, space before int

  • Merge branch 'master' into soc-2020-testing-frameworks
  • Added misisng space before parameter type in doc string
  • Resolved a merge conflict related to the threshold of Weld modifier
Himanshi Kalra (calra) marked 2 inline comments as done.Sep 27 2020, 1:05 PM

Tested again after latest patch

  • Resolved merge conflict, reformatted, break long lines, parameters

Latest change is about style only, so patch still LGTM

Campbell Barton (campbellbarton) requested changes to this revision.Dec 8 2020, 1:25 PM
Campbell Barton (campbellbarton) added inline comments.
tests/python/modifiers.py
65

Why was this line removed? it seems like useful information to keep.

tests/python/modules/mesh_test.py
280–328

Suggest to call _set_parameters_impl, (typically used when the internal implementation needs to be separate).

284

Should be dict or sequence ?

298–300

This exception seems like something we should avoid, as-is, it seems fairly harmless.
OTOH, if more similar cases are added here it could easily become a mess.

Suggest to be more specific in this case, this could check the modifier type too for example.

303

It's not obvious which attribute is expected to fail (although I'd assume setattr(modifier, setting, modifier_parameters)).

Is there any reason the try/except can't be replaced with a call to hasattr?

This is normally preferable as try/except that contain more than a short snippet of code can hide mistakes/errors.

317

Can be written as: if nested_settings_path:

325

This isn't a copy and it seems the newly defined variable doesn't serve any purpose, should it be removed.

332

This can be a tuple (preferred if it's not modified).

334

Any reason to set the frame to zero instead of 1? (as was done previously).

Suggest to use 1 since it's more common, or add a comment why zero is important.

461–466

This exceptions could hide useful information, suggest including the exception when re-raising. eg:

except TypeError as ex:
    raise TypeError(
        "Incorrect operator parameters {!r}, "
        "raised exception {!r}".format(operator.operator_parameters, ex))

Same for _apply_operator_object_mode.

513

Was this left in on purpose?

538

Was this left in on purpose?

581

The check could be inverted, using an early return instead, seeing as the result is to skip all comparison code.

641

Name is misleading, this raises an exception when not unique.

Could be called: _ensure_unique_test_name_or_raise_error

651–653

Enumerate here seems redundant, looping on the value would make more sense.

695–700

This block of code reads strangely.

  • initializing case to the first test doesn't seem necessary, instead it could be initialized to None, then if count == len_test can be replated with a check for case is None
  • count variable isn't needed, index can be used instead.
  • enumerate could use a value instead of _ as the value is accessed multiple times.
tests/python/physics_particle_instance.py
50

str() is redundant here.

This revision now requires changes to proceed.Dec 8 2020, 1:25 PM
Himanshi Kalra (calra) marked 13 inline comments as done.
  • Merge branch 'master' into soc-2020-testing-frameworks
  • Post Review: fix coding styles, remove try..except, using value for enumerate
  • Merge branch 'master' into soc-2020-testing-frameworks
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Dec 15 2020, 6:18 PM
  • Updated failing tests for Weld,Surface Deform and ParticleSystemTest and ParticleInstanceSimple

Particle System Test was failing due to change in frame from 0 to 1, rest two, not have a good idea.

tests/python/modifiers.py
65

This line was removed because I was able to reproduce it. There is a test for Skin in Line 225-227 in modifiers.py

tests/python/modules/mesh_test.py
298–300

I am not sure how to handle it in a better way? since modifier has become a setting, I can't access modifier.type anymore. IMO we can keep it like this. I am up for suggestions to make it better.

325

The newly defined variable modifier_copy is sent to set_parameters_impl because the modifier is changed to become a setting. Are you suggesting that the function creates a copy within its scope and the original modifier is intact?

538

Yes, for formatting reasons (output on the console)

Besides the minor issue of modifier_copy (replied inline) seems fine now.

tests/python/modules/mesh_test.py
325

In this case assigning the copy won't do what I think you intend it to do. As assigning it to a new variable wont make a copy. This would only make sense if you overwrote the modifier variable and wanted to access the previous value.

This revision is now accepted and ready to land.Dec 16 2020, 7:09 AM
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)
  • Removed unused variable modifier_copy

Quickly checked here on linux again, all tests pass fine, so LGTM.

Note for committing this to master: I'd recommend to update the tests in svn repo first, just before pushing changes in git master branch (reason is, our new devops intend to use timestamps to match tests version from svn with a specific commit in git master, for the buildbots and CI stuff, so tests files should ideally be committed before code changes that require them).

  • Updated frame end value for Build modifier, now shows non-empty mesh in blend file
  • Disabling particle instance modifier test
  • Merge branch 'master' into soc-2020-testing-frameworks
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Dec 17 2020, 1:15 PM

The test files have been updated, Particle Instance file has been reverted back to the previous version (this test has been disabled though)
Modifier blend file has been updated with Build modifier usages.