In many unit tests (e.g., in dbaccess/qa/extras/empty-stdlib-save.cxx), we have this pattern asserting on return value of MacrosTest::loadFromDesktop: > uno::Reference<lang::XComponent> xComponent = loadFromDesktop(aFileName); > CPPUNIT_ASSERT(xComponent.is()); MacrosTest::loadFromDesktop itself asserts on its return value (see [1]). Thus, there additional checks in unit tests are redundant, and only create noise unrelated to the tested functionality. This easy hack is about finding all the places where the method is used, and removing the direct assert on its return value if present. [1] https://opengrok.libreoffice.org/xref/core/unotest/source/cpp/macros_test.cxx?r=042033f1&mo=984&fi=39#39
Hey, can I work on this?
(In reply to Mike Kaganski from comment #0) > In many unit tests (e.g., in dbaccess/qa/extras/empty-stdlib-save.cxx), we > have this pattern asserting on return value of MacrosTest::loadFromDesktop: > > > uno::Reference<lang::XComponent> xComponent = loadFromDesktop(aFileName); > > CPPUNIT_ASSERT(xComponent.is()); > > MacrosTest::loadFromDesktop itself asserts on its return value (see [1]). > Thus, there additional checks in unit tests are redundant, and only create > noise unrelated to the tested functionality. > > This easy hack is about finding all the places where the method is used, and > removing the direct assert on its return value if present. > > [1] > https://opengrok.libreoffice.org/xref/core/unotest/source/cpp/macros_test. > cxx?r=042033f1&mo=984&fi=39#39 Hey, can I work on this? (Sorry, for making the comment twice. still getting used to the UI)
(In reply to Moaz El-defrawy from comment #1) > Hey, can I work on this? Welcome! Of course, please do. Everyone is welcome; this task may be done by multiple developers in parallel :-)
Update: I am removed 99% of the redundant asserts and I will submit a patch very soon. I had a question though: There is a pattern of asserting the return values of many functions not just "loadFromDesktop". Why don't we assert their return values instead just like "loadFromDesktop" and remove the extra ones? another question: I am not sure how style the comments to make them more readable. I tried html and readme files styling but neither worked.
(In reply to Moaz El-defrawy from comment #4) > Update: I am removed 99% of the redundant asserts and I will submit a patch > very soon. Very good! > I had a question though: > There is a pattern of asserting the return values of many functions not just > "loadFromDesktop". Why don't we assert their return values instead just like > "loadFromDesktop" and remove the extra ones? This needs discussing case-by-case. In this form, I can't answer this. Which specific change(s) you propose? > another question: > I am not sure how style the comments to make them more readable. I tried > html and readme files styling but neither worked. Are you talking about commit message, or about some comments in code? It's best to discuss in Gerrit change, where you post some proposed change, and then comment on parts that you are unsure with, and get responses from reviewers. Looking forward to see the change on Gerrit. Thanks for working on this!
(In reply to Mike Kaganski from comment #5) > (In reply to Moaz El-defrawy from comment #4) > > Update: I am removed 99% of the redundant asserts and I will submit a patch > > very soon. > > Very good! > > > I had a question though: > > There is a pattern of asserting the return values of many functions not just > > "loadFromDesktop". Why don't we assert their return values instead just like > > "loadFromDesktop" and remove the extra ones? > > This needs discussing case-by-case. In this form, I can't answer this. Which > specific change(s) you propose? I was thinking about it as a general principle and not about specific changes. but, I just realized you I am incorrect. For example, what if we want the function to return 0 or null at some cases. > > > another question: > > I am not sure how style the comments to make them more readable. I tried > > html and readme files styling but neither worked. > > Are you talking about commit message, or about some comments in code? It's > best to discuss in Gerrit change, where you post some proposed change, and > then comment on parts that you are unsure with, and get responses from > reviewers. I am sorry for not being clear enough. I was asking if there is a way to style the comments here (in this page) in case I wanted to insert some code. it seems like '>' character is the only way to do that? > > Looking forward to see the change on Gerrit. Thanks for working on this! Thanks a lot. I am excited to finish it and move on to the next issue.
(In reply to Moaz El-defrawy from comment #6) > > > I am not sure how style the comments to make them more readable. I tried > > > html and readme files styling but neither worked. > > > > Are you talking about commit message, or about some comments in code? It's > > best to discuss in Gerrit change, where you post some proposed change, and > > then comment on parts that you are unsure with, and get responses from > > reviewers. > > I am sorry for not being clear enough. I was asking if there is a way to > style the comments here (in this page) in case I wanted to insert some code. > it seems like '>' character is the only way to do that? No problem; so the answer is: Bugzilla is *not* the place to discuss code. Gerrit is :-)
Moaz committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/a9a350c13d8fb454c3f1db7ca6a7f10c1e7a19bb tdf#139734 Remove redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Hi, I submitted a patch for this. https://gerrit.libreoffice.org/c/core/+/111810 But the build by jenkins is failing, although make check was successful on my system. Can you please review it?
Moaz committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/dbbda04d52e75858840ce9748a19a61a1c565149 tdf#139734 Remove redundant asserts after functions loadFromDesktop and load It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
dipanshu124 committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/993298cfff01e09142c98faf03c3dbb608369114 tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Hi, I'm new to open source contributions. This whole forums looks too cryptic to me, where can I find the code to try to contribute to it? Any advice for a newbie would be greatful. Ashwin
(In reply to Ashwin from comment #12) > I'm new to open source contributions. > This whole forums looks too cryptic to me, where can I find the code to try > to contribute to it? > > Any advice for a newbie would be greatful. First of all, it is assumed you have the source code on your own computer and have followed https://wiki.documentfoundation.org/Development/GetInvolved One aspect of this task is to exercise your searching skills and imagination. As this seems to be getting solved completely soonish, I guess more clues can be revealed. At least this quick and dirty grep revealed some remaining ones: git grep -A 2 loadFromDesktop *qa/*.cxx Tweak & optimise the search by using your imagination.
Forgot to note that you should be careful not to step on the toes of others by looking at the currently unmerged patches: https://gerrit.libreoffice.org/q/message:139734+status:open
Ahmet Hakan Çelik committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c1d10276d32cbdaecf6894be723ab8d29cc7dbe2 tdf#139734: Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
sarynasser committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/8ed68179e2eaf07289663c64d923d8b5dd8b25c9 tdf#139734 remove redundant asserts after loadFromDesktop It will be available in 7.2.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
The CPPUNIT_ASSERT_MESSAGE Macros don't seem to throw an error when given a false value while testing the function from which loadFromDesktop() is called. Are these tests not supposed to run when the function they're in is called from someplace else?
(In reply to Sabyasachi Bhoi from comment #17) > The CPPUNIT_ASSERT_MESSAGE Macros don't seem to throw an error when given a > false value while testing the function from which loadFromDesktop() is > called. Are these tests not supposed to run when the function they're in is > called from someplace else? Please be specific. Your description does not allow to guess what specifically you do, and hence why you see what you see. We can't know *how* you are "testing the function from which loadFromDesktop() is called", and what "someplace else" is meant. It's best to have a gerrit change with specific changes there, and inline comments, instead of patch-related discussion here in Bugzilla. Thanks for looking into this :)
Hey, I tried submitting my first patch (https://gerrit.libreoffice.org/c/core/+/120319). I ran make check successfully on my system but the build by Jenkins failed. Could you please review it?
(In reply to Karan Abrol from comment #19) Hi! Thanks for working on this. I have resumed the build that apparently failed for an unrelated reason. An advise/reminder to everyone: please add the person who had created the easy hack (i.e., who had set the relevant flag in the bug, and provided the code pointers) as a reviewer in the gerrit change. All related discussion should happen there at gerrit, not here :)
Karan Abrol committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/7b21c25354d16b9d9e04f36c1370b9a2edfe09fc tdf#139734 Remove redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
KaranAbrol committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/c9bcbebe34c8d0e3edab520ace15c4d367aef62f tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Yildiray committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/08058894d2da9e65b91a1dcdb997b70f69448fb3 tdf#139734:Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Radhey Parekh committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/b3f3f0cb2a5ea7873349f733b55e41c1cbc671dc tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
4k5h1t committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/7970e700f86bca811508f9b229110d6ab4393d04 tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Henrik Palomäki committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/0935a53a1ef70d0cbaef5fbfe2a0bc2f3ddfeafd tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.3.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
yalda committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/e9fce801c2a888b76fd8230343b04f72b5b6b5ce tdf#139734 deleting redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
I'm currently working on this easyhack. And I came across some instances where a call to CPPUNIT_ASSERT_MESSAGE was made checking mxComponent.is(). Isn't CPPUNIT_ASSERT_MESSAGE is also redundant? Or I should not touch it ?
(In reply to comment #28) "This easy hack is about finding all the places where the method is used, and removing the direct assert on its return value if present." The idea is: remove all of them, be it 'assert' from <cassert>, or CPPUNIT_ASSERT, or CPPUNIT_ASSERT_MESSAGE, or even plain 'if (!foo)' or 'if (foo)'. One should understand, that if the situation that '!foo' is impossible, it's equally useless to use any kind of checks that test it.
pragat-pandya committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/73ef2f6f615b88ef25366f9073c5fb3db5e76687 tdf#139734 removing redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Are there any more of these left?
(In reply to Hannah Meeks from comment #31) Only one :-)
Siddhant Chaudhary committed a patch related to this issue. It has been pushed to "master": https://git.libreoffice.org/core/commit/43f6ca7164a9ea7c99d177d711684e25d53e8be4 tdf#139734 Drop redundant asserts after MacrosTest::loadFromDesktop It will be available in 7.4.0. The patch should be included in the daily builds available at https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More information about daily builds can be found at: https://wiki.documentfoundation.org/Testing_Daily_Builds Affected users are encouraged to test the fix and report feedback.
Thanks everyone participating in this! Let's call it fixed. If someone manages to catch some missing bit, it would be a rare and endangered species ;)