[prev in list] [next in list] [prev in thread] [next in thread] 

List:       koffice-devel
Subject:    Re: ODF text (was Release schedule after the alpha's)
From:       Sebastian Sauer <mail () dipe ! org>
Date:       2007-12-23 17:03:49
Message-ID: 200712231803.49963.mail () dipe ! org
[Download RAW message or body]

Thomas Zander wrote:
> On Thursday 20 December 2007 02:00:14 Sebastian Sauer wrote:
>> Thomas Zander wrote:
>> > I think having unit tests for loading is an absolute must, but every
>> > time I try I'm lost in the amount of classes that have to be
>> > instantiated to be able to actually call something like
>> > KoTextLoader::loadList
>>
>> y, I also agree there. Well, an excuse may that we did follow the
>> XP-development but did left out the chapter that deals with unittests
>> ;)
>>
>> > I'd like to suggest two things
>> >
>> > 1) refactor the context objects to have no arguments to their
>> > constructors unless they are required for the context objects
>> > constructor to run. Add setFoo() methods on those classes.
>>
>> sounds for me more like a cosmetic change which could be for sure done
>> rather fast but may not provide a the best solution :-/
> 
> From reading your whole mail, I think its not cosmetic, its due to
> different approaches to testing. And how what I've learned to do testing
> its very important to have a very cheap way to instantiate a class with
> the option to set extra objects in there, as is required for the test.
> I'll explain more in depth what I mean later on, but I just wanted to
> point out that this change in code makes the code more easily unit
> tested. (read on for why)

well, the KoTextLoadingContext.h class at least contains a small sample how to 
use it :)

>> > And then almost all methods on KoTextLoader you get have to pass in
>> > an instance of KoTextLoadingContext. The one you created *with* the
>> > loader as a reference.
>>
>> Nah, and that's where I did look at the code :) and...
>> seems it's not used at all.
> 
> Low hanging fruit, always good. And good to know my brief inspection of
> the code has already yielded one optimization.

After re-looking more detailed at the code, we have atm following case;

The TextLoader instance itself needs to be passed around between the different
loadOdf(const KoXmlElement & element, KoShapeLoadingContext & context)
calls. For that case TextLoaderContext that inherits KoOasisLoadingContext got 
introduced and holds a pointer to the KoTextLoader + to the KoStyleManager 
since it's needed to pass both on.

I don't see there a better way then subclassing KoShapeLoadingContext or 
KoOasisLoadingContext and attach there such state-details during loading.

>> > Bottom line; I want to have a way to test this.  In my test i need a
>> > way to instantiate the correct classes.
>> > Define an XML that I then let KoXml load, as my testing code.
>> > Then I want to call specific methods in the KoTextLoader and check if
>> > the QTextDocument looks correct afterwards.
>> >
>> > Who can help me with that? I already added an empty test framework to
>> > be filled ;)
> 
> On Thursday 20 December 2007 02:13:36 Sebastian Sauer wrote:
>> > please just commit it, so I can discover probably other points that
>> > are not logical / missing :)
>>
>> Thinking again about it; such a unittest-suite would probably also link
>> against KWord since kotext does not contain all the code needed to
>> load+display a odt-document (it misses all those parts that are within
>> kword/part/KWOpenDocumentLoader.* ). So, to "check if the QTextDocument
>> looks correct afterwards" I would suggest to just use kword ;) imho a
>> "real"(TM) unittest would need to do it like the KHTML-testsuite does.
> 
> This is now what I had in mind when I said I wanted to unit test.
> In my years of writing unit tests I noted that its very useful to make the
> unit of testing as small as possible. Typically one method in a class.
> The idea is to make sure you execute a minimum amount of code outside of
> that class while doing your testing to make sure you are testing *only*
> the coding-unit, and not more.
> To do what you write above requires instantiating a lot more classes, and
> running through more code that will hopefully call the method you want to
> test. Which means that if in the basic loading code something fails; a
> lot of tests fail. Which is a bit counter productive as you then still
> have to guess where in the code something broke.
> 
> Well, I guess if you go to websites that discuss XP they can explain it
> much better than I can.
> 
> So, my idea of unit testing the ODF-text loading code is to setup a
> minimum of data in each test. Typically just an xml stream and the
> context as well as a QTextDocument, and then call the loadList() method
> directly. For example.
> Afterwards you can inspect the data classes to see if the right thing
> happened.
> 
> So, for that reason I want to
> a) find the code sequence fully typed out in one standalone file that
> allows me to call the loadFoo()

Propably the KoTextShapeData::loadOdf() method does provide here an idea how 
it may could be done. Not sure there yet if we should refactor it before.

> b) refactor the code (like the constructor stuff at the top of this mail)
> to allow me to instantiate less classes against which I'll test.

It should be already possible. Well, thinking again about all this, I guess 
the only real solution would be to move the frame-code from KWord to kotext 
what would allow us to merge all th KWOpenDocumentLoader code into kotext as 
well. Sounds like a bigger task :-/
 
> So, if you can help out with (a) at least, then I'd love to do that
> couple-of-hours-of-refactoring soon as well :)
> 
> Lets make this code that's fun to hack on!
_______________________________________________
koffice-devel mailing list
koffice-devel@kde.org
https://mail.kde.org/mailman/listinfo/koffice-devel
[prev in list] [next in list] [prev in thread] [next in thread]