Bug 90290 - reduce some copy&paste code in ucalc
Summary: reduce some copy&paste code in ucalc
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Calc (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Stefan Weiberg
URL:
Whiteboard: target:5.2.0 target:5.4.0
Keywords: difficultyBeginner, easyHack, skillCpp
Depends on:
Blocks:
 
Reported: 2015-03-28 02:29 UTC by Markus Mohrhard
Modified: 2017-04-16 21:53 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
Fix Bug 90290 (177.12 KB, text/x-c++src)
2015-03-31 10:15 UTC, Tapan Prakash T
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Mohrhard 2015-03-28 02:29:40 UTC
ucalc is one of the mail calc tests and is split between many files in sc/qa/unit

This task is about improving the existing pattern around checkFormula in sc/qa/unit/ucalc_formula.cxx

A typical piece of code looks like this:

    if (!checkFormula(*m_pDoc, aPos, "SUM(A3:A7)"))
        CPPUNIT_FAIL("Wrong formula!");

This should be changed to something like:

checkFormula(*m_pDoc, aPos, "SUM(A3:A7)", "Wrong formula!");
Comment 1 Markus Mohrhard 2015-03-28 02:57:00 UTC
And inside of the checkFormula implementation we should use CPPUNIT_ASSERT_EQUAL_MESSAGE which will print the expected and actual string in case of a failure.
Comment 2 Tapan Prakash T 2015-03-31 10:15:01 UTC
Created attachment 114489 [details]
Fix Bug 90290
Comment 3 Markus Mohrhard 2015-03-31 10:25:13 UTC
(In reply to Tapan Prakash T from comment #2)
> Created attachment 114489 [details]
> Fix Bug 90290

Please use gerrit. See https://wiki.documentfoundation.org/Development/gerrit


The attached file is not even a patch so it is unreviewable.
Comment 4 Michael Stahl (CIB) 2015-03-31 10:33:35 UTC
Comment on attachment 114489 [details]
Fix Bug 90290

mark not-a-patch as obsolete, please use gerrit
Comment 5 Tapan Prakash T 2015-03-31 14:54:23 UTC
How to I submit patch for this specific bug.
Comment 6 Tapan Prakash T 2015-03-31 14:55:28 UTC
Any branch name convention?
Comment 7 Kohei Yoshida 2015-03-31 15:41:16 UTC
BTW, just to mention this for the record, I did this type

if (!checkFormula(*m_pDoc, aPos, "SUM(A3:A7)"))
    CPPUNIT_FAIL("Wrong formula!");

of checks so that when it fails, you can get the line number of where the check actually fails.  One disadvantage of writing a common function for this is that when the check fails, the line number now points where inside that shared function, not where it was called, and then you'd have to do some hunting to see which check actually failed in what test case.

I'm not saying "don't do it", but I'm simply saying there is a thought behind it, and there is a disadvantage to "cleaning it up".
Comment 8 Kohei Yoshida 2015-03-31 16:33:23 UTC
Anyway, once again, my comment is just for informational purposes.  It is by no means meant to discourage the cleanup proposed here. :-)
Comment 9 Brent Ritzema 2015-04-11 04:39:37 UTC
I noticed this was a quick fix so I submitted a patch... Sorry if I invaded your space Tapan. I just figured this might help you more than hurt you.
Comment 10 Tor Lillqvist 2015-04-11 08:10:27 UTC
Re: comment #7, maybe it might be a good idea to add API to cppunit (we are its upstream now, aren't we?) that also take an explicit line number, so that the

checkFormula(*m_pDoc, aPos, "SUM(A3:A7)", "Wrong formula!");

that Markus gives as example in the initial comment could look like:

checkFormula(*m_pDoc, aPos, "SUM(A3:A7)", "Wrong formula!", __LINE__);

Brent: no need to be sorry, it is against our policy to assign bugs to yourself, but people (especially newbies, and especially GSoC hopefuls) keep doing it anyway. (The problem with it, as can perhaps be seen here (although it is not even two weeks yet), is that people often assign a bug to themselves, do a failed attempt, lose interest, but the bug stays "assigned" and thus scaring other potential volunteers away.) Or Tapan, have you made any progress?
Comment 11 Brent Ritzema 2015-04-11 08:14:40 UTC
Okay thank you for the information. However, I am also a GSOC hopeful haha :). I have submitted a patch however I can submit another one that will use your suggestions. I just want to get something in soon for the GSOC qualifications.

On a side note, I am a bit confused by "maybe it might be a good idea to add API to cppunit". If I could get more information on this that would be great.
Comment 12 Tor Lillqvist 2015-04-11 08:26:03 UTC
Brent: The line number and file name in messages about failed unit tests come (in the examples in the initial comment) from where the CPPUNIT_FAIL macro is called. So if CPPUNIT_FAIL is moved into some function that is called from multiple places, the message no longer gives the true line number and source file to identify which test actually fails. Thus it might be useful if there was another macro, CPPUNIT_FAIL_AT(message, file, line), and such a function (the checkFormula() in Markus's comment) would then take also file name and line number, which would be passed on to CPPUNIT_FAIL.

CPPUNIT_FAIL (and other macros starting with "CPPUNIT_") are from a separate piece of software called cppunit. As it happens, it's the LibreOffice developers (Markus in particular, I think) that are the upstream nowadays of that software, so we can easily amend its API as we see useful, and release a new version. Brent, you could even suggest such a patch to cppunit. (I am not sure what the correct procedure for that is, do we even have gerrit for that, too?)
Comment 13 David Tardon 2015-04-11 15:42:33 UTC
(In reply to Markus Mohrhard from comment #0)
> ucalc is one of the mail calc tests and is split between many files in
> sc/qa/unit
> 
> This task is about improving the existing pattern around checkFormula in
> sc/qa/unit/ucalc_formula.cxx
> 
> A typical piece of code looks like this:
> 
>     if (!checkFormula(*m_pDoc, aPos, "SUM(A3:A7)"))
>         CPPUNIT_FAIL("Wrong formula!");
> 
> This should be changed to something like:
> 
> checkFormula(*m_pDoc, aPos, "SUM(A3:A7)", "Wrong formula!");

What is wrong with

CPPUNIT_ASSERT_MESSAGE("Wrong formula!", checkFormula(*m_pDoc, aPos, "SUM(A3:A7)"))

? That would address Kohei's concerns without any necessary changes to cppunit.
Comment 14 Markus Mohrhard 2015-04-13 19:41:25 UTC
(In reply to David Tardon from comment #13)
> (In reply to Markus Mohrhard from comment #0)
> > ucalc is one of the mail calc tests and is split between many files in
> > sc/qa/unit
> > 
> > This task is about improving the existing pattern around checkFormula in
> > sc/qa/unit/ucalc_formula.cxx
> > 
> > A typical piece of code looks like this:
> > 
> >     if (!checkFormula(*m_pDoc, aPos, "SUM(A3:A7)"))
> >         CPPUNIT_FAIL("Wrong formula!");
> > 
> > This should be changed to something like:
> > 
> > checkFormula(*m_pDoc, aPos, "SUM(A3:A7)", "Wrong formula!");
> 
> What is wrong with
> 
> CPPUNIT_ASSERT_MESSAGE("Wrong formula!", checkFormula(*m_pDoc, aPos,
> "SUM(A3:A7)"))
> 
> ? That would address Kohei's concerns without any necessary changes to
> cppunit.

I already talked to Kohei and he seems to be fine with changing it.

The reason that I wanted to move to CPPUNIT_ASSERT_EQUAL is that it will print both strings in case of a failure which is extremely helpful for debugging.

I think if necessary we can implement Tor's suggestion in LibreOffice. There is no need to do it in CPPUNIT as there should be already support for an own asserter message.
Comment 15 Brent Ritzema 2015-04-15 04:34:06 UTC
I have submitted a patch to gerrit for this, however I will amend what I did to accommodate these new suggestions. I will use CPPUNIT_ASSERT_MESSAGE instead. However I will be gone these next few days, so I hope to get it done sometime within the next week or so.
Comment 16 David Tardon 2015-04-15 06:54:45 UTC
(In reply to Brent Ritzema from comment #15)
> I have submitted a patch to gerrit for this, however I will amend what I did
> to accommodate these new suggestions. I will use CPPUNIT_ASSERT_MESSAGE
> instead.

Please do not do this. Instead, make checkFormula a custom assertion, as Markus suggested. See http://cppunit.sourceforge.net/doc/cvs/struct_asserter.html for details.
Comment 17 Commit Notification 2015-06-19 11:52:36 UTC
ritztro committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ab9663ed187ba4776d9ff3d9c21f83eed478a268

tdf#90290 reduce some copy&paste code in ucalc.

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 18 Commit Notification 2015-06-19 12:41:52 UTC
Eike Rathke committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b36dbb499e19fe8a497a48d7c78ad3cb5d40a83d

Revert "tdf#90290 reduce some copy&paste code in ucalc."

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 19 Robinson Tryon (qubit) 2015-12-13 10:58:06 UTC Comment hidden (obsolete)
Comment 20 Commit Notification 2016-02-03 20:01:06 UTC
Markus Mohrhard committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=aaa7b7c87dc0d685e2a0edf2763aaeacd2d6e579

tdf#90290, add custom asserter for formula

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Robinson Tryon (qubit) 2016-02-18 14:51:49 UTC Comment hidden (obsolete)
Comment 22 Stefan Weiberg 2016-04-15 14:11:34 UTC
I am going to work on this easy hack. I will use Markus Mohrhard's custom asserter.
Comment 23 jani 2016-04-15 15:10:15 UTC
(In reply to Stefan Weiberg from comment #22)
> I am going to work on this easy hack. I will use Markus Mohrhard's custom
> asserter.

Please assign it to yourself, so others can see you are working on it. We use assign for easyHacks, so that assigned bugs do not appear in the lists.

have fun
jan i
Comment 24 Stefan Weiberg 2016-04-18 13:28:14 UTC
Uploaded a patch to gerrit for review:
https://gerrit.libreoffice.org/#/c/24215/
Comment 25 Commit Notification 2016-04-18 15:53:41 UTC
Stefan Weiberg committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=54a547dbc157d66d5977f2e078655c5d4a03a420

tdf#90290: use custom asserter for formula check

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 26 jani 2016-05-20 05:51:56 UTC
Looks like this bug can be closed.
Comment 27 Commit Notification 2017-01-04 01:50:04 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9fb55f7b1de8be1c3f86a4ae540f648574d3ed45

tdf#90290 use custom asserter in filters-test.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 28 Commit Notification 2017-01-04 15:03:09 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bddfa273edc7591d7a5e171e1abab035fe42716f

tdf#90290 use custom asserter in subsequent_export-test.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 29 Commit Notification 2017-01-05 21:32:38 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1b60db958f84c61066ea30e143a4f33423a657d9

tdf#90290 use custom asserter in subsequent_filters-test.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 30 Commit Notification 2017-01-08 02:14:32 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=9aea56370ae78a43c3543159571ea0454c1619eb

tdf#90290 use custom asserter in ucalc_sort.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 31 Commit Notification 2017-01-08 03:39:34 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=4d968302479531f7b9ba0f9c35b2efe0dbe39f50

tdf#90290 use custom asserter in ucalc_sharedformula.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 32 Commit Notification 2017-01-08 13:06:55 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c00f6dd0a39c295c702dcfc808f009f1cdc365f0

tdf#90290 use custom asserter in ucalc.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 33 Commit Notification 2017-03-15 09:22:02 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=1238cd22c4e2e801eab612d667038e0173b4dccd

tdf#90290 use custom asserter in ucalc_formula.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 34 Commit Notification 2017-03-28 20:48:24 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=837bf336cbcc7ddb4b07146cc7f654d5ea013cb1

tdf#90290 use custom asserter in ucalc_sharedformula.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 35 Commit Notification 2017-03-28 20:52:52 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=cc020cc261abc4866f3104b274705cbc86002898

tdf#90290 use custom asserter in shared_test_impl.hxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 36 Commit Notification 2017-04-16 21:53:16 UTC
kerem committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=0497facb723d90fae6cade86d9075ddfc18f83b9

tdf#90290 use custom asserter in ucalc_sort.cxx

It will be available in 5.4.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.