Page MenuHome

Prototype for generating test objects for Regression testing frameworks(GSOC)
Needs ReviewPublic

Authored by Himanshi Kalra (calra) on Jun 6 2020, 8:12 PM.

Details

Summary

The script can be used to create

  • test and expected objects with the given names
  • generate vertex groups based on the vertex index specified

Diff Detail

Event Timeline

Himanshi Kalra (calra) requested review of this revision.Jun 6 2020, 8:12 PM
Himanshi Kalra (calra) created this revision.
Himanshi Kalra (calra) retitled this revision from Prototype for generating test objects for Regress testing frameworks(GSOC) to Prototype for generating test objects for Regression testing frameworks(GSOC).

Coding style:

  • Have a look at Blender's coding style and revise your code
  • No documentation and 0 comments is usually bad. A basic doc string per function would really help understand what you are trying to achieve and how users are supposed to use your script.

General approach:

  • Why not merge creating a vertex group and creating a test object in a single function?
  • We discussed the option to create a random vertex group. Any plans to add that?
  • I'm not strongly against global variables but here it's probably best to create a class that stores all internal parameters/states in one place. So a bit similar to 'MeshTest', where you have a class that does the actual work and another class that stores specifications.
tests/python/modules/test_object_generator.py
17–19

This is redundant. You already have max value so just check if max is larger than the allowed maximum. You can do the same for negative values with min(..)

22

the function is called do_selection but it also assigns vertex group? You should avoid this for two reasons:

  1. Name of function should be accurate
  2. A function should do 1 thing and 1 thing only, otherwise you confuse people using your function with unwanted side effects.
36

what if the object does not exist?

41

You should validate function input before doing anything, otherwise the function exists with an exception with the job half way done. You also probably don't need an extra function to validate input. Just write the validation code at the beginning of this function

58–63

Not sure why you need a function here, you are wrapping only 1 line?

78–80

You don't have to duplicate this code for every obj_type, just write it once after all the if statments

141–148

This has to be removed. The script must have a well defined list of arguments. The user as well as the underlying function can 'agree' on what the arguments mean beforehand. Ideally you should print a help message about how the script is supposed to be used, what it does and so on...

158

The file will get created even when there are errors later on. Is this intentional?

It's probably best to either create a file that has been generated correctly or don't create anything at all, right?

Habib Gahbiche (zazizizou) requested changes to this revision.Jun 6 2020, 8:56 PM
This revision now requires changes to proceed.Jun 6 2020, 8:56 PM

Addressed the comments

Himanshi Kalra (calra) marked 8 inline comments as done.Wed, Jun 10, 10:59 PM

I want to suggest to move on as soon as the script does what it's supposed to do in a correct and stable manner. Otherwise we could be improving the script forever without gaining much.

tests/python/modules/test_object_generator.py
33

It looks like you are passing an object and not a mesh, so argument name seems wrong...

38

Use True/False instead

140

This is one more reason why a class would be better here, where you save all internal states within the class instead of having to declare global variables.

Also, flag is not descriptive enough. Something like success is more commonly used

161

is conversion to str really necessary? I thought it could work without it..

171–172

Does this really work? flag is set inside the function create_test_objects() but it only gets checked before that function is called...

Added exception safety and some minor improvements.

Himanshi Kalra (calra) marked 4 inline comments as done.Thu, Jun 11, 9:36 PM
Himanshi Kalra (calra) added inline comments.
tests/python/modules/test_object_generator.py
161

yes

171

This one's extra, and will be removed in the next commit.

tests/python/modules/test_object_generator.py
32

You were right I think, object is a keyword in Python and it's probably best to choose something else.