search

ddobrev / QtSharp

Mono/.NET bindings for Qt
Other
571 stars 52 forks source link

QtSharp tests #10

Closed KeKl closed 9 years ago

KeKl commented 9 years ago

Tests marked as done in ToDo.txt are done. Other test classes are in work or bigger bugs are in the bindings. Tests marked with [Ignore("Bug!)] cant be executed because of bugs in the binding (freezing of vs).

ddobrev commented 9 years ago

Thank you. Here's my review:

  1. At quite a few places, such as QBasicTimerTests.TestEmptyConstructor, you allocate an object with new () and then test it for non-null. Unlike other languages, such as Objective-C, C# can never return null from a constructor. Therefore such tests do not verify anything and are therefore pointless. Please delete them but note that I mean just new () constructs - testing a property or a method for non-null is perfectly fine;
  2. TestAboutToQuit - I don't know what the bug is there but I can see you quit before attaching the event, your test would never run like this;
  3. QDateTests.TestGreaterEqual and many other places - you use IsTrue for comparison. Please stick to AreEqual, AreGreater, AreLess, because those report more info on failure. Another note on QDate, QUrl, QFileInfo, QUuid and other classes with system equivalents - having them tested is great but in the long term I'd like to map them to their system counterparts. So if I delete those tests one day because the relevant types are no longer part of the API, there won't be any hard feelings, right? :)
  4. QFileInfoTests.TestEmptyConstructorNotThrowingAnException and similar - I don't see any issue with such tests but I think I should share with you an idea I've had for a long time about these types of tests. They are easy to auto-generate, that is, have some tool inspect all wrapped types through reflection, get their members, and generate a C# source file with tests that just call those members. Of course, the approach has some limitations, such as the inability to mock all, if any, required parameters but it could save some work. Anyway, it's just an idea I've had, I have no problem taking your manual tests but I just thought you may like to hear it.

I think that is all. Once 1-3 are fixed, I'll merge it.

KeKl commented 9 years ago
  1. is done.
  2. Youre right. But there is another bug.
  3. is done, if applicable.
ddobrev commented 9 years ago

Merged. Thank you very much for this large effort.