Page MenuHome
Differential D10865

Operator test for average_normals
Needs RevisionPublic
Actions

Authored by Akshat Dixit (:baka) (aaka) on Mar 31 2021, 10:09 AM.

Details

Reviewers
Habib Gahbiche (zazizizou)
Maniphest Tasks
T84999: Add Operator tests
Summary

operators.blend3 MBDownload

The new operator.blend file to include this test.

Operator tests added for -

- average_normals - Cube
- beautify_fill - Text (8gwx)
- spin - Cube - Vertex mode
- Subdivide - Icosphere

Diff Detail

Repository
rB Blender
Branch
T84999 (branched from master)
Build Status
Buildable 13836
Build 13836: arc lint + arc unit

Event Timeline

Akshat Dixit (:baka) (aaka) requested review of this revision.Mar 31 2021, 10:09 AM
Akshat Dixit (:baka) (aaka) created this revision.
Harbormaster completed remote builds in B13836: Diff 35795.Mar 31 2021, 10:10 AM
Akshat Dixit (:baka) (aaka) edited the summary of this revision. (Show Details)Mar 31 2021, 10:13 AM
Pratik Borhade (PratikPB2123) awarded a token.Mar 31 2021, 10:19 AM
Habib Gahbiche (zazizizou) requested changes to this revision.Mar 31 2021, 7:28 PM

Thanks! I think the patch is good and the test case is useful. Just a few comments regarding style:

  • we should keep objects in blend file organized alphabetically, so new object has to be first. Note that position does not matter, so changing positions of other objects won't make the test fail.
  • follow similar naming convention for test names, so start with object type followed by test case.
This revision now requires changes to proceed.Mar 31 2021, 7:28 PM
Akshat Dixit (:baka) (aaka) added a comment.Mar 31 2021, 7:58 PM
In D10865#276922, @Habib Gahbiche (zazizizou) wrote:

Thanks! I think the patch is good and the test case is useful. Just a few comments regarding style:

  • we should keep objects in blend file organized alphabetically, so new object has to be first. Note that position does not matter, so changing positions of other objects won't make the test fail.
  • follow similar naming convention for test names, so start with object type followed by test case.

Thankyou, I will do those changes and some more tests to same patch as well. (Was waiting for any suggestions!)

Habib Gahbiche (zazizizou) added a comment.Mar 31 2021, 8:57 PM

Thankyou, I will do those changes and some more tests to same patch as well. (Was waiting for any suggestions!)

I have no specific suggestions, we want to cover everything in that list eventually. It's probably best to address consecutive tests from that list though to avoid moving objects around too much...

Akshat Dixit (:baka) (aaka) updated this revision to Diff 35836.Apr 2 2021, 9:45 AM

Added few more tests

Harbormaster completed remote builds in B13857: Diff 35836.Apr 2 2021, 9:45 AM
Akshat Dixit (:baka) (aaka) edited the summary of this revision. (Show Details)Apr 2 2021, 9:46 AM
Habib Gahbiche (zazizizou) mentioned this in T84999: Add Operator tests.Apr 2 2021, 1:29 PM
Habib Gahbiche (zazizizou) accepted this revision.Apr 2 2021, 1:34 PM
Habib Gahbiche (zazizizou) added a reviewer: Bastien Montagne (mont29).

Thanks. Tests look fine to me. I'd prefer a better description for the revision though, e.g. which tests have been added and how many objects.

Adding Bastien as reviewer as all commits need to be reviewed by a module owner.

Himanshi Kalra (calra) added a subscriber: Himanshi Kalra (calra).Apr 2 2021, 2:11 PM
Himanshi Kalra (calra) added inline comments.
tests/python/operators.py
50–72

The idea is to add the smallest possible test case that could make the test fail. Unless you are covering some special case and think test might fail with a lot of faces (then do mention it)
Also from a quick glance, it looks they are in sequence being incremented by 1. You could use a list comprehension approach {i for i in range(472)}

Akshat Dixit (:baka) (aaka) edited the summary of this revision. (Show Details)Apr 2 2021, 2:34 PM
Akshat Dixit (:baka) (aaka) added inline comments.Apr 2 2021, 2:37 PM
tests/python/operators.py
50–72

I did not get the fail thing you mentioned but tests with beautify fill was working good. I had to find a decent test case where decent amount of faces were changing with beautify_fill that is why I went for this test case, If you think I should change of something please mention!

Akshat Dixit (:baka) (aaka) added a comment.Apr 6 2021, 11:25 AM

@Bastien Montagne (mont29) Request for a review!

Habib Gahbiche (zazizizou) requested changes to this revision.Sep 5 2021, 12:32 PM
Habib Gahbiche (zazizizou) removed a reviewer: Bastien Montagne (mont29).

Can you also rebase on latest master?

tests/python/operators.py
50–72

Here, you are selecting all faces, so use {i for i in range(472)} instead of the long list of numbers as suggested.

This revision now requires changes to proceed.Sep 5 2021, 12:32 PM

Revision Contents
Changeset List

PathSize
tests/
python/
5 lines

Diff 35795

View Options

tests/python/operators.py

Loading...