Bug 139734 - Drop redundant asserts after MacrosTest::loadFromDesktop
Summary: Drop redundant asserts after MacrosTest::loadFromDesktop
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: low normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.2.0 target:7.3.0 target:7.4.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2021-01-18 09:22 UTC by Mike Kaganski
Modified: 2022-05-31 09:08 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Kaganski 2021-01-18 09:22:00 UTC
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
Comment 1 Moaz El-defrawy 2021-02-05 03:16:34 UTC
Hey, can I work on this?
Comment 2 Moaz El-defrawy 2021-02-05 03:17:38 UTC Comment hidden (obsolete)
Comment 3 Mike Kaganski 2021-02-05 04:32:47 UTC
(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 :-)
Comment 4 Moaz El-defrawy 2021-02-19 21:28:00 UTC
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.
Comment 5 Mike Kaganski 2021-02-20 04:08:51 UTC
(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!
Comment 6 Moaz El-defrawy 2021-02-21 17:02:00 UTC
(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.
Comment 7 Mike Kaganski 2021-02-21 17:06:23 UTC
(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 :-)
Comment 8 Commit Notification 2021-02-25 06:51:30 UTC
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.
Comment 9 Dipanshu Garg 2021-03-04 07:41:15 UTC
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?
Comment 10 Commit Notification 2021-03-05 07:38:04 UTC
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.
Comment 11 Commit Notification 2021-03-14 19:24:44 UTC
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.
Comment 12 Ashwin 2021-03-22 06:40:37 UTC
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
Comment 13 Buovjaga 2021-03-22 07:12:36 UTC
(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.
Comment 14 Buovjaga 2021-03-22 07:13:54 UTC
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
Comment 15 Commit Notification 2021-04-23 05:46:29 UTC
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.
Comment 16 Commit Notification 2021-06-09 17:44:59 UTC
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.
Comment 17 Sabyasachi Bhoi 2021-07-23 08:00:21 UTC
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?
Comment 18 Mike Kaganski 2021-07-23 08:46:09 UTC
(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 :)
Comment 19 Karan Abrol 2021-08-11 14:17:59 UTC
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?
Comment 20 Mike Kaganski 2021-08-11 14:21:34 UTC
(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 :)
Comment 21 Commit Notification 2021-08-11 15:27:35 UTC
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.
Comment 22 Commit Notification 2021-08-13 08:35:22 UTC
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.
Comment 23 Commit Notification 2021-08-18 11:36:39 UTC
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.
Comment 24 Commit Notification 2021-08-26 14:07:25 UTC
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.
Comment 25 Commit Notification 2021-08-30 14:23:35 UTC
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.
Comment 26 Commit Notification 2021-11-02 16:02:47 UTC
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.
Comment 27 Commit Notification 2022-01-26 01:45:47 UTC
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.
Comment 28 Libreoffice user SSO 2022-02-28 08:55:16 UTC
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 ?
Comment 29 Mike Kaganski 2022-02-28 09:09:53 UTC
(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.
Comment 30 Commit Notification 2022-02-28 21:22:49 UTC
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.
Comment 31 Hannah Meeks 2022-04-16 16:13:33 UTC
Are there any more of these left?
Comment 32 Mike Kaganski 2022-04-16 18:23:15 UTC
(In reply to Hannah Meeks from comment #31)

Only one :-)
Comment 33 Commit Notification 2022-05-26 05:52:05 UTC
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.
Comment 34 Mike Kaganski 2022-05-31 09:08:52 UTC
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 ;)