#273 [Patch] Unit test for nmg_copy

Incomplete
closed-accepted
None
5
2014-05-19
2014-05-15
Zhao Anqing
No

Hi, Daniel

I submit another patch about the unit test of nmg_copy. The basic process of it is as following:

  • Build some model as test case.
  • Call nmg_clone_model(...) to get a copy.
  • Check the consistency of two model (original model and the copy).

I try my best to demonstrate my understanding of the HACKING rules learned in the previous patch. Hope it's satisfying.

What's more, I write some compare functions for each level of nmg struct. According to the suggestion by Sean, I will move them into an independent file such as nmg_compare.c and perfect it (now, it's not complete) then.

As for the test result, you can see the 'testcase 3' fail because the 'nmg_clone_model' changes the order of regions in new model's r_hd. Append each region in the head of the list, so the order is reversed.(I guess it's for the performance.) It seems no problem in logic but I am not quite sure.

Thank you!

Zhao Anqing

1 Attachments

Discussion

  • Zhao,

    Your patch is almost perfect. By reading through it I couldn't find any issue. Then I tried to compile it and got an error:

    /home/rossberg/Devel/BRL-CAD/brlcad/src/librt/tests/test_nmg_copy.c: In function ‘testcase3’:
    /home/rossberg/Devel/BRL-CAD/brlcad/src/librt/tests/test_nmg_copy.c:442:23: error: variable ‘r1’ set but not used [-Werror=unused-but-set-variable]
    struct nmgregion *r1;

    This are the kind of problems you only find with the GNU compiler. Unfortunately this is the one most users are using. That's why you should test every non-trivial change with it.

    BTW, removing r1 there and ignoring the return of nmg_mrsv(m1) will fix the error.

    While you're at it: Is it possible to change "test-case-x" to something more meaningful?

    Regarding test case 3: The copy function shouldn't change the order of regions. Even if it isn't a change in the logic nmg_clone_model() should be fixed to produce real 1-to-1 copies. (Unfortunately I wrote this function ;)

    Daniel

     
  • Zhao Anqing
    Zhao Anqing
    2014-05-17

    Daniel, Thanks for the review. I made some change according to your suggestions.

    • remove the unused variable 'r1'.
    • change testcase's name to a more meaningful one.

    Is that true I should write another patch if I want to correct the copy function? I remember your instruction that each patch should just do one thing.

    Thank you!

    Zhao Anqing

     
    • status: open --> closed-accepted