[Merge] lp:~phablet-team/qt-halide/unit-tests into lp:qt-halide

Florian Boucault florian.boucault at canonical.com
Wed Mar 25 12:55:25 UTC 2015


Review: Needs Fixing

6) class Test : public HalideGeneratorExtended<Test> is poorly named
In fact all these test generators should probably not have their name prefixed with 'Test'

7) set_packed is only necessary for the AOT generators, for JIT generators it is done dynamically

8) backend/modules/Filters/test_helper.cpp, Qt includes should contain the module name, e.g. #include <QtCore/QStandardPaths>

9) is there any way to have a test/check target and possibly a coverage target appearing in QtCreator?

10) why not use compare or fuzzyCompare (e.g.compare(image.pixel(20, 20), Qt.rgba(255, 0, 0, 255)) instead of creating our own Util.checkColor?

11) 'checkColor' would be better and more consistently named 'comparePixelWithColor'

12) function checkPixels(r, g, b, a, msg) is duplicated code and should go in Util

13) checkPixels naming does not indicate what it does

14) function waitForUpdated(spy, test, work, updateCount, msg) is mixing up lots of different use cases:
  - what it really does is waiting for 'updateCount' signal emissions. As such it should be a new method of SignalSpy, for example function waitNemissions(n, timeout), doable by creating a subclass of SignalSpy
  - In cases where only 1 emission is expected there is no need to actually use this method, but the more traditional and documented approach of spy.wait(); compare(spy.count, 1); in other cases spy.waitNemissions(n); compare(spy.count, n) should do just fine
  - Now if we want to make it a tiny bit simpler we could create a SignalSpy class inheriting from that custom one dedicated to monitoring 'updatingChanged' signals. It would have a method that does the wait and the compare.
  - Finally in that last class, we could add method dedicated to setting 'functions' on HalideTransform and therefore deduce 'n' automatically

-- 
https://code.launchpad.net/~phablet-team/qt-halide/unit-tests/+merge/254064
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/qt-halide/unit-tests.



More information about the Ubuntu-reviews mailing list