Added simple unit test sample for demonstration #4154

burrima · 2020-01-20T18:59:13Z

based on cmocka (https://cmocka.org) - thus, it needs to be installed before building. On Ubuntu 18.04: "apt install libcmocka-dev"
build darktable with option -DBUILD_TESTING=On
then run 'make test' in the build directory

Pull request contains 2 commits that were made earlier by @cryptomilk - thank you very much for your help!

cryptomilk

Please split the patches into smaller ones, the test itself looks fine for me.

cmake/modules/AddMockedTest.cmake

src/CMakeLists.txt

src/common/darktable.h

aurelienpierre · 2020-01-22T11:37:42Z

Thanks a lot for this, that was so so so much needed ! I will let @cryptomilk do the review.

cryptomilk · 2020-01-22T11:48:01Z

FYI: I will give a talk about cmocka at devconf.cz this weekend. I guess it is recorded and will be available next week.

TurboGit · 2020-01-23T06:59:11Z

That's a wonderful step forward! Thanks a lot, for sure this will help dt to avoid regressions and will make big refactoring less scary! Looking forward to have this in action !

src/tests/unittests/CMakeLists.txt

src/tests/unittests/test_filmicrgb.c

cryptomilk · 2020-01-24T20:11:01Z

I've checked what the issue is and it is rawspeed which uses BUILD_TESTING. So it is turned off for rawspeed but this turns it off globally.

burrima · 2020-01-24T20:22:52Z

I've checked what the issue is and it is rawspeed which uses BUILD_TESTING. So it is turned off for rawspeed but this turns it off globally.

Not sure if you are right. My code is listening to BUILD_TESTS and not BUILD_TESTING. But I just realized that it works when I include cmocka in the base CMakeLists.txt.

cryptomilk · 2020-01-24T21:01:04Z

See https://cmake.org/cmake/help/latest/command/enable_testing.html

BUILD_TESTING is forced to OFF.

burrima · 2020-01-25T16:02:07Z

See https://cmake.org/cmake/help/latest/command/enable_testing.html

BUILD_TESTING is forced to OFF.

I see. To me, cmake is still very new. Do you have any suggestion on how to fix this issue? Current state of this PR is working fine but I think it only works by chance and it is not a proper solution (cmake should probably be included in ./src/CMakeLists.txt and not in ./CMakeLists.txt, right?).

cryptomilk · 2020-01-25T17:22:27Z

We need to change the variable in rawspeed to BUILD_RS_TESTING first. It shouldn't use BUILD_TESTING.

burrima · 2020-01-26T10:00:30Z

Ok, I think I understand now. I gave it a try and made a pull request for rawspeed: darktable-org/rawspeed#223
In this pull request here, I still need to update the rawspeed submodule ref as soon as above PR has merged.

LebedevRI · 2020-01-26T10:03:58Z

Please, please. When writing a commit message, don't just state "we need to do XYZ".
That may be FUD. State what the problem is, and why the particular solution may be the right fix.

burrima · 2020-01-26T12:52:23Z

Thank you, @LebedevRI, your comment made me re-think again. As far as I understand now, the BUILD_TESTING is set to Off for rawspeed - or more precisely: set to Off by the darktable build system in src/external/CMakeLists.txt. But it is done "hidden" - i.e. the original value of BUILD_TESTING is stored at the beginning and reverted at the end. Thus, I think the pull request in rawspeed is not needed.

Also, I found out that my first commit (fixing the include of ThreadSafetyAnalysis.h) is not needed anymore with the current implementation.

My current code is working fine on my machine (retried both cases with/without tests and deleted the buld dir each time before). I will update this PR. @cryptomilk could you please have a look again? Does it make sense to you as well?

LebedevRI · 2020-01-26T12:56:05Z

As far as I understand now, the BUILD_TESTING is set to Off by rawspeed - or more precisely: in src/external/CMakeLists.txt

Note that it is darktable's file, not rawspeed's.

But it is done "hidden" - i.e. the original value of BUILD_TESTING is stored at the beginning and reverted at the end.

Yep, at least that is the intent.

burrima · 2020-01-26T13:01:59Z

Note that it is darktable's file, not rawspeed's.

Yep, updated my above comment

cryptomilk · 2020-01-26T14:43:29Z

Ok, it looks like BUILD_TESTING is not a problem. I'm on the train home and finally tracked it down.

See here: https://github.com/cryptomilk/darktable/tree/master-fix

$ make test
Running tests...
Test project /home/asn/workspace/projects/darktable/obj
    Start 1: test_sample
1/2 Test #1: test_sample ......................   Passed    0.00 sec
    Start 2: test_test_filmicrgb
2/2 Test #2: test_test_filmicrgb ..............   Passed    0.05 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.05 sec

cryptomilk · 2020-01-26T15:00:19Z

@burrima Do you want to push my branch here?

burrima · 2020-01-26T16:30:34Z

@cryptomilk yes sure, thank you very much for your support.

Honestly I cannot follow what issue you are addressing with the extra commits. On my PC I get (almost) exactly the same output (without your fix):

burrima@vulcano:~/src/darktable/build$ make test
Running tests...
Test project /home/burrima/src/darktable/build
    Start 1: test_simplesample
1/2 Test #1: test_simplesample ................   Passed    0.00 sec
    Start 2: test_filmicrgb
2/2 Test #2: test_filmicrgb ...................   Passed    0.02 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.03 sec

Why do you get "test_test_filmicrgb" (note the double-"test_")?

cryptomilk · 2020-01-26T17:01:14Z

I've already fixed that (double test_). I've split the code a bit more and include(CTest) needs to be included in the main CMakeLists.txt else it doesn't work.

burrima · 2020-01-26T17:14:54Z

Did you get my latest code updates to this branch? - I already have the CTest included in the topmost CMakeLists.txt. The reason why I am asking is because I see that you still have an old state of test_fimlicrgb.c in your branch. I re-formatted it with clang and removed the ugly looking /*******... lines. Thus, just taking your branch is maybe not the best idea from my perspective.

I could include your 2 fixing commits before my commits if you wish so (though maybe a separate pull request for them would be more clever since I think the code is not related). But at the end, your voice has more power since you are in this project way longer than me :-)

BTW: how was your presentation? I looked through your slides on the devconf.cz site and found them very informative.

cryptomilk · 2020-01-26T17:39:50Z

Can you merge my work into yours? :-)

burrima · 2020-01-26T18:05:59Z

Yes, I'll do that. Sounds like the best strategy for me as well :-) Please give me some time since I'm going for dinner now.

burrima · 2020-01-26T20:17:57Z

Ok, done.

BTW: your branch master-fix does not build since there is still a test_ too much in src/cmake/modules/AddCMockaMockTest.cmake ;-)

cryptomilk · 2020-01-28T19:55:39Z

@burrima Could you add cryptomilk@c19aba4

cryptomilk · 2020-01-28T21:01:48Z

Please rebase on master and repush tomorrow. New docker images with cmocka should be available then ...

burrima · 2020-01-31T20:13:39Z

Ran docker on my ubuntu 18.04 pc. Then built dt inside the docker image. Tests then were running fine...
What I did in particular:
export SD=/home/burrima/src/darktable
sudo docker run -it --volume $SD:$SD:rw --workdir $SD darktable/darktable bash
cd build
export CC=/usr/bin/gcc-9
export CXX=/usr/bin/g++-9
cmake .. -DBUILD_TESTING=ON
cmake --build . -- -j6
ctest

cryptomilk · 2020-02-01T08:18:59Z

@LebedevRI Roman, I'm not really familiar with TravisCI, do you have an idea why the tests fail with "Permission denied"?

@burrima Maybe also print:

tail -100 /var/log/messages
tail -100 /var/log/secure
tail -100 /var/log/audit/audit.log

cryptomilk · 2020-02-01T08:42:24Z

@burrima Try with cryptomilk@7447782

cryptomilk · 2020-02-01T08:55:44Z

That's it. https://github.com/cryptomilk/darktable/commits/master-cmocka works :-)

This will work once we just allow to run darktable in the build dir. Dave is working on this.

- based on cmocka (https://cmocka.org) - thus, it needs to be installed before building. On Ubuntu 18.04: "apt install libcmocka-dev" - build darktable with option -DBUILD_TESTING=ON - then run `make test` Signed-off-by: Martin Burri <[email protected]>

This commit integrates function mocking with cmocka. The code is still very empty and I will continue to work on this. Signed-off-by: Martin Burri <[email protected]>

cryptomilk · 2020-02-01T12:36:16Z

@TurboGit This is good to go!

TurboGit · 2020-02-01T15:11:03Z

@burrima @cryptomilk : Maybe I expected something else so I'm not sure to understand what is provided here. Is that only unit test on the code ?

What I thought was worked on was a way to give a source image, some actions like an XMP apply it and check that the output is consistent what what is expected (fuzzy comparison with the target image). Is that covered here ?

Whatever the answer to the question above I think we want a README in the test directory explaining how to add a test ? I did not find the high level description about this and I do feel this very important as adding tests is something we want to see encouraged as much as possible.

burrima · 2020-02-01T15:37:53Z

@TurboGit This is code for function unit testing of the darktable code. I will add methods to create very simple test images in a next pull request. Thus, process() methods of modules can be tested. But unit-tested. So, no function testing as you described it.

A README is underway as well. Together with already implemented tests.

burrima · 2020-02-01T15:42:58Z

@TurboGit feel free to look at my work in progress. But please be gentle, it's really still under progress :-)

https://github.com/burrima/darktable/tree/test_filmicrgb

cryptomilk · 2020-02-02T07:31:33Z

@TurboGit Pascal, what you expected is called integration test, this is unit testing :-) Martin already found bugs in filmic rgb writing unit tests for it :-)

DefineOptions.cmake

burrima · 2020-02-04T19:35:50Z

Hi @TurboGit, may I kindly ask what makes you hold back this pull request? Besides of documentation (which I promised to deliver in my next pull request) what else is missing or making you struggle? - Or do you just need more time to review? I'm not in a hurry at all though it would be a great satisfaction to me to see my first real contribution to darktable going in. Best regards, Martin

TurboGit · 2020-02-04T20:12:57Z

@burrima : Just time and priorities. I've been working on integrating the new iop-order and working on big performance degradation. This PR is certainly a nice contribution, I just need more time to review and merge.

burrima · 2020-02-04T20:36:58Z

Sure, no problem, take your time. I was just wondering if you are expecting more from my side.

TurboGit · 2020-02-06T18:45:08Z

@burrima : I had a look and the code as-is is ok to me. I'm not really sure how easy or feasible it will be to create serious test. BTW, is that to test only iop code or also the other parts of dt ? And in this case how all this will interact with the database (many routines are doing SQL) ? The pixel-pipe ? Are all those parts covered here ?

If it is only iop I think it is "simpler" and still just for this it will be nice to have in.

I have then nothing against merging this, still please give me some feedback regarding my questions above. Thanks.

burrima · 2020-02-06T19:24:26Z

@TurboGit thank you very much for the review and your feedback. From my side, I will focus on the iop for a good while. But there is generally nothing against writing unit tests for other parts. The nice thing about cmocka is that you can "mock" (https://en.wikipedia.org/wiki/Mock_object) functions, meaning that you can "overwrite" functions that are called by the function-under-test to mimic different behavior. Thus, you can break dependencies (e.g. to the database etc.).

Unit tests are in my eyes just a start - there is definitely the need for more automated testing (e.g. when it comes to GUI interaction).

TurboGit · 2020-02-06T19:31:33Z

Ok, so let's move on ! I'm eager to see unit test be added :) Thanks for the work!

burrima · 2020-02-06T19:34:42Z

@TurboGit cool, thank you very much! Unit tests are in the pipe :-)

cryptomilk suggested changes Jan 22, 2020

View reviewed changes

cmake/modules/AddMockedTest.cmake Outdated Show resolved Hide resolved

src/CMakeLists.txt Outdated Show resolved Hide resolved

src/common/darktable.h Outdated Show resolved Hide resolved

TurboGit added this to the 3.2 milestone Jan 23, 2020

cryptomilk suggested changes Jan 23, 2020

View reviewed changes

src/tests/unittests/CMakeLists.txt Outdated Show resolved Hide resolved

src/tests/unittests/test_filmicrgb.c Outdated Show resolved Hide resolved

src/tests/unittests/test_filmicrgb.c Show resolved Hide resolved

src/tests/unittests/test_filmicrgb.c Outdated Show resolved Hide resolved

burrima mentioned this pull request Jan 26, 2020

Renamed option BUILD_TESTING to BUILD_RS_TESTING darktable-org/rawspeed#223

Closed

cryptomilk and others added 7 commits February 1, 2020 12:07

cmake: Remove unneeded include directories

ec83745

cmake: Remove uneeded properties from darktable-test-variables

db9cadd

This will work once we just allow to run darktable in the build dir. Dave is working on this.

src:tests: Added function mocking

b978d23

This commit integrates function mocking with cmocka. The code is still very empty and I will continue to work on this. Signed-off-by: Martin Burri <[email protected]>

ci: Update ci-script

d0ac74f

ci: Allow to execute tests on tmpfs

95bddc0

ci: Enable unit testing

f230751

TurboGit self-requested a review February 1, 2020 15:11

houz reviewed Feb 4, 2020

View reviewed changes

DefineOptions.cmake Show resolved Hide resolved

TurboGit removed the wip pull request in making, tests and feedback needed label Feb 6, 2020

TurboGit merged commit 1a7019e into darktable-org:master Feb 6, 2020

burrima deleted the mockasample branch February 6, 2020 19:33

johnny-bit mentioned this pull request Apr 25, 2020

Locallaplacian speedup 2 #4809

Merged

Added simple unit test sample for demonstration #4154

Added simple unit test sample for demonstration #4154

Conversation

burrima commented Jan 20, 2020 • edited Loading

cryptomilk left a comment

Choose a reason for hiding this comment

aurelienpierre commented Jan 22, 2020

cryptomilk commented Jan 22, 2020

TurboGit commented Jan 23, 2020

cryptomilk commented Jan 24, 2020

burrima commented Jan 24, 2020

cryptomilk commented Jan 24, 2020

burrima commented Jan 25, 2020

cryptomilk commented Jan 25, 2020

burrima commented Jan 26, 2020

LebedevRI commented Jan 26, 2020

burrima commented Jan 26, 2020 • edited Loading

LebedevRI commented Jan 26, 2020

burrima commented Jan 26, 2020 • edited Loading

cryptomilk commented Jan 26, 2020

cryptomilk commented Jan 26, 2020

burrima commented Jan 26, 2020 • edited Loading

cryptomilk commented Jan 26, 2020 • edited Loading

burrima commented Jan 26, 2020

cryptomilk commented Jan 26, 2020

burrima commented Jan 26, 2020

burrima commented Jan 26, 2020

cryptomilk commented Jan 28, 2020

cryptomilk commented Jan 28, 2020

burrima commented Jan 31, 2020

cryptomilk commented Feb 1, 2020

cryptomilk commented Feb 1, 2020

cryptomilk commented Feb 1, 2020

cryptomilk commented Feb 1, 2020

TurboGit commented Feb 1, 2020 • edited Loading

burrima commented Feb 1, 2020

burrima commented Feb 1, 2020

cryptomilk commented Feb 2, 2020

burrima commented Feb 4, 2020

TurboGit commented Feb 4, 2020

burrima commented Feb 4, 2020

TurboGit commented Feb 6, 2020

burrima commented Feb 6, 2020

TurboGit commented Feb 6, 2020

burrima commented Feb 6, 2020

burrima commented Jan 20, 2020 •

edited

Loading

burrima commented Jan 26, 2020 •

edited

Loading

burrima commented Jan 26, 2020 •

edited

Loading

burrima commented Jan 26, 2020 •

edited

Loading

cryptomilk commented Jan 26, 2020 •

edited

Loading

TurboGit commented Feb 1, 2020 •

edited

Loading