Re: Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/#review50388 --- Ship it! Ship It! - Aleix Pol Gonzalez On Feb. 20, 2014, 3:53 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- (Updated Feb. 20, 2014, 3:53 p.m.) Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- (Updated Feb. 20, 2014, 5:15 p.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/#review50389 --- This review has been submitted with commit 6d3a4405fc2723f5796e845247b6b0eb0448216e by Alex Richardson to branch master. - Commit Hook On Feb. 20, 2014, 3:53 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- (Updated Feb. 20, 2014, 3:53 p.m.) Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
On Feb. 17, 2014, 7:39 a.m., Matthew John Dawson wrote: autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp, line 26 https://git.reviewboard.kde.org/r/115634/diff/1/?file=243130#file243130line26 Is this a copy + paste error? Yes, leftover from old version of this unittest On Feb. 17, 2014, 7:39 a.m., Matthew John Dawson wrote: autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp, line 153 https://git.reviewboard.kde.org/r/115634/diff/1/?file=243130#file243130line153 As this appears to preform the same check as the function below, I rather just have the one set of tests. As the previous test breaks up the different elements under test such that Qt sees the different tests, I prefer for that version to be used. Doesn't quite do the same thing. This test checks that signals get emitted when using the generated setters. The other test uses the reflective approach with setProperty(). This test actually mostly works before https://git.reviewboard.kde.org/r/115635/ except for the last check with setDefaults(). The second test function only starts working after the patch to kconfig_compiler On Feb. 17, 2014, 7:39 a.m., Matthew John Dawson wrote: autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp, line 226 https://git.reviewboard.kde.org/r/115634/diff/1/?file=243130#file243130line226 As the solution for this problem ends up being a wrapper that works for every type that correctly implements the KConfigSkeletonItem contract, I don't think we need to test every possible type. Did you experience any particular issues with the other types? Otherwise I rather just test one type, which should help speed up the test suite. Do you mean every type of generated KConfigSkeleton (dpointer vs no dpointer and singleton vs no singleton) or every type of stored data (QString vs int, etc). I actually found a bug in kconfig_compiler due to testing every type: https://projects.kde.org/projects/frameworks/kconfig/repository/revisions/b1487f0c6b4ea64ae9c1ce96348fce8b5f0828df But in general there should be no difference depending on what gets saved. Maybe there should be another unit test that generates a class that uses config items of every possible type to make sure something like this does not happen and restrict the signals test to a single property. Don't really which solution gets chosen, but I guess I can then finally get rid of that ugly macro in testSetters - Alexander --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/#review50007 --- On Feb. 11, 2014, 1:47 a.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- (Updated Feb. 11, 2014, 1:47 a.m.) Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
On Feb. 17, 2014, 1:39 a.m., Matthew John Dawson wrote: autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp, line 153 https://git.reviewboard.kde.org/r/115634/diff/1/?file=243130#file243130line153 As this appears to preform the same check as the function below, I rather just have the one set of tests. As the previous test breaks up the different elements under test such that Qt sees the different tests, I prefer for that version to be used. Alexander Richardson wrote: Doesn't quite do the same thing. This test checks that signals get emitted when using the generated setters. The other test uses the reflective approach with setProperty(). This test actually mostly works before https://git.reviewboard.kde.org/r/115635/ except for the last check with setDefaults(). The second test function only starts working after the patch to kconfig_compiler Ah, I see. Can you merge the two sets of tests, so that they both use the data driven system? I just prefer the output Qt generates when that is used. On Feb. 17, 2014, 1:39 a.m., Matthew John Dawson wrote: autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp, line 226 https://git.reviewboard.kde.org/r/115634/diff/1/?file=243130#file243130line226 As the solution for this problem ends up being a wrapper that works for every type that correctly implements the KConfigSkeletonItem contract, I don't think we need to test every possible type. Did you experience any particular issues with the other types? Otherwise I rather just test one type, which should help speed up the test suite. Alexander Richardson wrote: Do you mean every type of generated KConfigSkeleton (dpointer vs no dpointer and singleton vs no singleton) or every type of stored data (QString vs int, etc). I actually found a bug in kconfig_compiler due to testing every type: https://projects.kde.org/projects/frameworks/kconfig/repository/revisions/b1487f0c6b4ea64ae9c1ce96348fce8b5f0828df But in general there should be no difference depending on what gets saved. Maybe there should be another unit test that generates a class that uses config items of every possible type to make sure something like this does not happen and restrict the signals test to a single property. Don't really which solution gets chosen, but I guess I can then finally get rid of that ugly macro in testSetters Sorry, I meant types of data stored. I think reducing this down to one single data type is best, as it avoids testing too many different ideas in one set of tests. In the future, it definitely would be good to have all the different types tested. I think it is good to test the different ways KConfigSekeleton is generated, as the type does change how signals are generated. - Matthew John --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/#review50007 --- On Feb. 10, 2014, 7:47 p.m., Alexander Richardson wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- (Updated Feb. 10, 2014, 7:47 p.m.) Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Review Request 115634: Add kconfig_compiler autotest that checks whether signals are emitted
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115634/ --- Review request for KDE Frameworks. Repository: kconfig Description --- Add kconfig_compiler autotest that checks whether signals are emitted Currently this works when using the setters, however when using setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no signals are emitted. Diffs - autotests/kconfig_compiler/CMakeLists.txt a2ebb9453bacb2c7507bc9477b6753a34bbcd434 autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp PRE-CREATION autotests/kconfig_compiler/signals_test.kcfg PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton.kcfgc PRE-CREATION autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc PRE-CREATION Diff: https://git.reviewboard.kde.org/r/115634/diff/ Testing --- Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass. Rather ugly code IMO, open for suggestions to improve it... Thanks, Alexander Richardson ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
