- User Since
- Feb 21 2015, 11:51 PM (295 w, 6 d)
Sun, Oct 11
Tested again after latest patch
Sun, Oct 4
Addressed comments from code review:
- changed test naming convention
Mon, Sep 28
Sun, Sep 27
Added more tests
Thu, Sep 24
Sep 13 2020
I changed the approach completely. Please let me know if you think this is the right way to test the compositor. If so, I will invest more time in generating test cases (should get more meaningful, faster, more representative of real world usage etc..).
@Germano Cavalcante (mano-wii) Ok thanks for checking.
Sep 8 2020
Patch looks good to me.
Hi @Jacques Lucke (JacquesLucke), could you say which os you tried this on? When building with make developer most (python) unit tests fail for me on mac and I believe they fail on linux too. (For context, this was discovered when testing D8507 with different make configurations)
Sep 6 2020
Aug 26 2020
Aug 22 2020
To summarize the comments:
- Each class/function should have a single responsibility
- GenObject should take care of object and file creation, whereas TestObject only holds creation specs
Aug 21 2020
The patch looks good to me. I think some test cases can be improved (see comments), and helper classes can be generalised (see quote).
Aug 12 2020
Aug 11 2020
As discussed, the overall structure of the framework looks fine to me. So far I looked at the code only and the comments are mostly about the framework user's perspective, since we already discussed the implementation in previous reviews.
Aug 7 2020
The special case in dfs is bothering me. The whole purpose of that function was to apply nested settings in a generic way... So have a closer look and see if you can get rid of it. I wouldn't waste too much time on it though, it's fine if there is no other way
Aug 6 2020
I think the change is fine. Just a few more comments or documentation to explain what's going on would be helpful.
Aug 5 2020
Jul 30 2020
Jul 24 2020
As discussed already, you could generalize PhysicsSpec instead. This patch will be revisited when the framework for all physics modifiers are done.
Curve Hook test does not seem to have any effect, so the test might never fail. Here is what you can do to verify if the modifier will have an effect (taking Hook as a an example from the attached file):
Jul 19 2020
Overall structure is fine. Following is missing:
- More test cases: a couple per modifier where at least 1 corner case and 1 typical use case is tested.
- Put more emphasis on newer modifiers, in this case weld
- Deform modifiers for curve are missing
Jul 12 2020
Structure looks good overall. Will need some more testing and polishing before merging to master, e.g. review documentation, all corner cases and add more (useful) tests.
Jul 11 2020
Didn't look at everything yet, I'll continue tomorrow ;)
Jul 6 2020
Pretty much all comments are related to self.apply_object_operator (for now), so please have a closer look at that.
Jun 22 2020
Thanks for the feedback, Campbell. This was indeed meant to facilitate code reviews related to automated testing.
Jun 21 2020
Jun 16 2020
Jun 13 2020
Have you tested this with a real test blend file?
Jun 12 2020
Jun 11 2020
Ok fine by me, as long as the test tackles the cause of the bug (which I think it is the case here).
It also follows some prior naming in this test (to reference bug reports) and in the ffmpeg and cycles tests which name tests and .blend files with bug numbers as well.
Cycles tests are nicely displayed in an html page so you can have a good overview of the tests without looking at names. Here it's not the case, we rely on descriptive names for the overview of (variations of) modifiers
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.
Jun 6 2020
- 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.
May 31 2020
Thanks! I tested it and it looks good to me. Although I'm not sure about the object names. There are plans to identify tests by name and not by index such that object names won't be important anymore.
May 25 2020
I can't reproduce this on macOS with AMD graphics card
May 7 2020
Apr 20 2020
Hi @Howard Trickey (howardt), any thoughts on this? Could you commit it if it look fine to you?
Apr 10 2020
Although this goes slightly beyond the strict definition of a regression test, I think it's useful to double check the newly updated geometry is valid.
Mar 27 2020
Actually I was discussing just that with @Himanshi Kalra (calra). But we might need a few more adjustments to make the framework usable as a performance test.
Mar 24 2020
That actually looks nice. I just saw that the code is in the branch performance-test. I would very much like to contribute to it. What I would like to do next is:
If I understood correctly, a performance test could like the following:
Mar 23 2020
Thanks Brecht. Could you commit the change? I don't have commit access...
rename thresh -> threshold in Python API
Mar 22 2020
Feb 9 2020
Jan 25 2020
I don't see any test that cannot be reproduced reliably and the tests do fail when the expected object is changed, so the changes look good to me.
Jan 19 2020
I think it's better to first merge D6576 though since it has larger changes.
Jan 18 2020
Patch looks good to me except for some minor issues (see inlined comments).
Thanks for the patch. It looks good to me except for minor styling issues.
From a first look, the tests look fine and they do indeed seem to cover relevant tests cases, so thanks for the patch (I haven't verified them in detail though, e.g. whether theses test can indeed fail of if for some reason one will always pass even if it shouldn't etc...). However, I do have some comments:
Jan 11 2020
Commented out test 0 because it does not seem to be reproducible.
Jan 9 2020
- fixed failing tests caused by a change in the offset's default value of the bevel modifier
- raise AttribiteException instead of printing an error message when a modifier's property does not exist.
Dec 24 2019
Dec 18 2019
I'm having a hard time trying to reuse the testing and reporting files for cycles and eevee. I get the error 'No render result found' and I don't know how to generate the renders, so I could really use some help here.
Dec 4 2019
Dec 2 2019
Did you see the way how we do regression test for Cycles? It was also used for Eevee (although, due to specifics of render on GPU those are not enabled by default yet). I would imagine Compositor can use same exact approach.
No, but I will have a closer look at those tests and see what I can reuse for compositor. Thanks for the hint.
To me it seems easier to understand what's going on when you can actually open .blend file and inspect it rather than trying to visualize the graph in your mind when reading code.
Might be true for complex node trees (then you could just execute the test with the option to open blender for inspection), but for simple one node tests, you only have to read the node's name and its parameters (so 1 line of "code").
Thanks for the feedback.
It was intentional to use code to assemble a node tree. The idea behind it is to make it possible to generate a test case using one line of code (e.g. compositor.py line 35. I tried to explain the idea behind the framework in T71834.
and also doesn't allow to catch possible issues in versioning code when node behaviour changes.
These are meant to be regression tests, so if a node behavior changes, (e.g. output or API change) then the test will fail, since it always compares the rendered image with a reference image.
Dec 1 2019
I'm not sure I'm doing the steps correctly but I can't reproduce the crash. I added this test case to D5357 (test currently passes with thickness = 0.4 and use_relative_offset set to True)
Cover test case from T71971
Rendered images are now written to disk and read again to compare them with expected result.
Thanks for reporting. I can't reproduce this on macOS with today's master (last commit is f1ac64921b49eaea8658d144754a1a532198c720).
Nov 30 2019
Can we first agree that this patch produces the expected behavior before continuing with the review of the code itself? For instance the check for rotation was introduced and the check for odd/even numbers of points was eliminated.
That "Affect Only" refers to tools, like when you press 'G' to grab or 'R' to rotate. The transform panel refers to the object as a whole. That's why this is not a bug.
The patch is almost done, but I would appreciate some help regarding these issues:
- Blank render when blender is ran in background mode: Nodes are created and linked correctly but blender always renders a blank image. Saving the blend file (after running in background) shows no background image. Selecting the same input image (again!) seems to refresh the compositor and give correct results again
- Beautify tree not working: this issue is less important (see comment in line 279)
Nov 24 2019
Nov 23 2019
- Changed related task
- Added task number to title. Tip: when the title of the revision/commit starts with 'Fix TXXXX' the bug will automatically get closed when the patch is commited
- Added reviewer to task
Nov 17 2019
- added deform modifiers tests
- updated modifiers.blend