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!");
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.
Created attachment 114489 [details] Fix Bug 90290
(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 on attachment 114489 [details] Fix Bug 90290 mark not-a-patch as obsolete, please use gerrit
How to I submit patch for this specific bug.
Any branch name convention?
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".
Anyway, once again, my comment is just for informational purposes. It is by no means meant to discourage the cleanup proposed here. :-)
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.
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?
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.
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?)
(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.
(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.
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.
(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.
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.
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.
Migrating Whiteboard tags to Keywords: (easyHack, difficultyBeginner, skillCpp) [NinjaEdit]
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.
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC) [NinjaEdit]
I am going to work on this easy hack. I will use Markus Mohrhard's custom asserter.
(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
Uploaded a patch to gerrit for review: https://gerrit.libreoffice.org/#/c/24215/
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.
Looks like this bug can be closed.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.