[Kde-pim] Review Request 109325: fixing tests in messageviewer
Sandro Knauß
mail at sandroknauss.de
Thu Mar 7 15:15:52 GMT 2013
> On March 7, 2013, 6:36 a.m., Laurent Montel wrote:
> > messageviewer/objecttreeparser.cpp, line 3111
> > <http://git.reviewboard.kde.org/r/109325/diff/1/?file=117580#file117580line3111>
> >
> > Before to fix objecttreeparser which is a special class, you need to show a bug report about it.
> > you replace a += to = it will change a lot of thing I think. so you need to show bug about it thanks.
> >
>
> Sandro Knauß wrote:
> The tests themself:
> http://build.kde.org/job/kdepim_master/1212/testReport/junit/(root)/TestSuite/messageviewer_objecttreeparsertest/
> [...]
> Actual (otp.plainTextContent()): This is the encapsulating message.This is the encapsulating message.
>
> Expected (QString( "This is the encapsulating message." )): This is the encapsulating message.
> [...]
> I think it is not intended to repeat the message.
>
> No it isn't used a lot:
> cd kdepim && grep -rl 'plainTextContent\W'
>
> messageviewer/objecttreeparser.cpp
> messageviewer/objecttreeparser.h
> messageviewer/tests/unencryptedmessagetest.cpp
> messageviewer/tests/objecttreeparsertest.cpp
> composereditor-ng/composereditor.cpp ( not related )
> composereditor-ng/composereditor.h ( not related )
> messagecomposer/composerviewbase.cpp ( no tests available )
> templateparser/templateparser.cpp: ( tested via templateparsertest )
> templateparser/tests/templateparsertest.cpp: QVERIFY( !otp.plainTextContent().isEmpty() );
> blogilo/src/postentry.h ( not related )
> blogilo/src/postentry.cpp ( not related )
>
> So there are one two places, where it used
> * Unfortuantelly templateparsertest is broken *grmpf*
> * I can't say what will be happen is messagecomposer/composerviewbase.cpp.
>
>
>
> Laurent Montel wrote:
> For me fix and potential break kmail/messageviewer to fix a unittest is not correct.
> until now nobody reported a bug about it.
> I prefere a unittest broken that a messageviewer/messagecomposer broken.
> until you prouve that it's correct and not break it, I don't want that you commit it.
> Regards.
>
> Sandro Knauß wrote:
> I just want to mention the documentation of ObjectTreeParser::plainTextContent(). It should not return the text of the message twice.
> /**
> * The text of the message, ie. what would appear in the
> * composer's text editor if this was edited or replied to.
> * This is usually the content of the first text/plain MIME part.
> */
> QString plainTextContent() const { return mPlainTextContent; }
>
> I'll try to make sure that messagecomposer/composerviewbase.cpp and templateparser/tests/templateparsertest.cpp won't break.
Ok now have looked up the chain form templateparser to objecttreeparser changes. There is only one path to objecttrreparser. Here are the relevat lines:
TemplateParser::TemplateParser
mEmptySource = new MessageViewer::EmptySource; //htmlWriter() = 0
mEmptySource->setAllowDecryption( mAllowDecryption );
mOtp = new MessageViewer::ObjectTreeParser( mEmptySource );
TemplateParser::processWithTemplate
mOtp->parseObjectTree( mOrigMsg.get() );
TemplateParser::plainMessageText
QString result = mOtp->plainTextContent();
Because htmlWriter() is 0, ObjectTreeParser::writeBodyString won't let us to ObjectTreeParser::writeBodyStr, where the changes of mPlainTextContent are:
void ObjectTreeParser::writeBodyString( const QByteArray & bodyString,
const QString & fromAddress,
const QTextCodec * codec,
ProcessResult & result,
bool decorate )
{
// FIXME: This is wrong, it means that inline PGP messages will not be decrypted when there is no
// HTML writer. Even if there would be a HTML writer, the decrypted inline PGP text is not
// added to the textual content.
// The solution would be to remove this if statement and make writeBodyStr() add the
// decrypted string to the textual content as well, and removing any manual modifictions
// of the textual content by callers of this method.
kDebug() << "writeBodyString";
if ( !htmlWriter() )
return;
[...]
}
For me this patch can't harm templateparser.
Same is true for messagecomposer/composerviewbase.cpp:
void Message::ComposerViewBase::updateTemplate ( const KMime::Message::Ptr& msg )
{
// First, we copy the message and then parse it to the object tree parser.
// The otp gets the message text out of it, in textualContent(), and also decrypts
// the message if necessary.
KMime::Content *msgContent = new KMime::Content;
msgContent->setContent( msg->encodedContent() );
msgContent->parse();
MessageViewer::EmptySource emptySource;
MessageViewer::ObjectTreeParser otp( &emptySource );//All default are ok
otp.parseObjectTree( msgContent );
// Set the HTML text and collect HTML images
if ( !otp.htmlContent().isEmpty() ) {
m_editor->setHtml( otp.htmlContent() );
emit enableHtml();
collectImages( msg.get() );
} else {
m_editor->setPlainText( otp.plainTextContent() );
[...]
}
Here the relevat part of messageviewer/objecttreeemptysource.cpp:
HtmlWriter * EmptySource::htmlWriter()
{
return 0;
}
-> My patch at objecttreeparser is harmless: qed
- Sandro
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109325/#review28745
-----------------------------------------------------------
On March 6, 2013, 11:33 p.m., Sandro Knauß wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109325/
> -----------------------------------------------------------
>
> (Updated March 6, 2013, 11:33 p.m.)
>
>
> Review request for KDEPIM.
>
>
> Description
> -------
>
> fixed messageviewer-objecttreeparsertest
> fixed messageviewer-unencryptedmessagetest
>
> one problem is still occures: gpgsm will open a pinentry window that has to be closed via mouse click.
>
>
> Diffs
> -----
>
> messageviewer/objecttreeparser.cpp d41e77f65adc20374b2ec358a80d9aebf6e9fed4
> messageviewer/tests/objecttreeparsertest.cpp ded141a6444a98392096984ad8f29fc4ea2b2210
> messageviewer/tests/unencryptedmessagetest.cpp 6b3dc7649b171df1746a8b43842da0690127a6bb
>
> Diff: http://git.reviewboard.kde.org/r/109325/diff/
>
>
> Testing
> -------
>
> cd messageviewer && make test
> Running tests...
> Test project /home/kdetest/kde/build/kdepim/messageviewer
> Start 1: messageviewer-htmlquotecolorertest
> 1/5 Test #1: messageviewer-htmlquotecolorertest ..... Passed 0.61 sec
> Start 2: messageviewer-objecttreeparsertest
> 2/5 Test #2: messageviewer-objecttreeparsertest ..... Passed 0.54 sec
> Start 3: messageviewer-rendertest
> 3/5 Test #3: messageviewer-rendertest ...............***Failed 0.99 sec
> Start 4: messageviewer-unencryptedmessagetest
> 4/5 Test #4: messageviewer-unencryptedmessagetest ... Passed 2.79 sec
> Start 5: accessiblemailwebviewtest
> 5/5 Test #5: accessiblemailwebviewtest .............. Passed 0.41 sec
>
> 80% tests passed, 1 tests failed out of 5
>
> Total Test time (real) = 5.36 sec
>
> The following tests FAILED:
> 3 - messageviewer-rendertest (Failed)
>
>
> one problem is still occures: gpgsm will open a pinentry window that has to be closed via mouse click.
>
>
> Thanks,
>
> Sandro Knauß
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list