Adding VCL GUI Logger #147
Conversation
…echnologies/master
Added VCL GUI Logger, implemented LogLevel all the way to ITestResult, moved exceptions to separate file, added "comparable" format support. Also added some tests to show off features in GUI, a UI test example, and a few "comparable format" examples.
|
This is failing on our build server, we build against XE7 (because that's what we are using in house), so will need to look at why. It's going to take a while to code review this, @rlove @staticcat @sglienke can you guys take a look at this too? I want to make sure this doesn't break TestInsight as that is used by a lot of people now, and if it does break it, to co-ordinate a release that deals with the change. Also, embarcadero ship fmx gui loggers, so need to see if those still work too. |
| class property TestFailure: ExceptClass read fTestFailure write fTestFailure; | ||
| class property TestPass: ExceptClass read fTestPass write fTestPass; | ||
| // class property TestFailure: ExceptClass read fTestFailure write fTestFailure; | ||
| // class property TestPass: ExceptClass read fTestPass write fTestPass; |
There was a problem hiding this comment.
If these are no longer in use should remove them instead of commenting out. Also this seems to be moving away from allowing the implementation of an external exception class by the user. Previously you could go: Assert.TestFailure := MyDecendantOfEAbort;
There was a problem hiding this comment.
Sorry, those must've slipped by me...
|
Understood - I was actively using TestInsight from XE and did tinker with the FMX logger. Is the FMX logger the one in this repo or do they have another one? Just curious because the one in the repo just didn't seem completely fleshed out. Hindsight being 20/20, next time I will separate changes from new stuff as I think it would be much easier to review that way. |
| procedure TDUnitXGUIVCLRichEditLogger.OnEndSetupFixture(const threadId: TThreadID; const fixture: ITestFixtureInfo); | ||
| begin | ||
| Outdent(1); | ||
| //WriteBlankLine; |
|
It's failing the CI build because of this - W:\CI_WS\Ws\737927\Source\DUnitX\Tests\DUnitXTest_XE7.dproj" (Build target) (1) -> You will need to remove any references to that unit from any dpr and dproj files. Just commit and push again and it should update this PR. |
| @@ -584,22 +602,6 @@ TDUnitX = class | |||
| function GetReport: string; | |||
| end; | |||
|
|
|||
|
|
|||
| ETestFrameworkException = class(Exception); | |||
There was a problem hiding this comment.
From memory these exception classes needed to be in TestFramework so that they would be included when please included DUnitX.TestFramework. This might break some peoples usage of DUnitX.
There was a problem hiding this comment.
This was key to getting the "comparable" exception to work. When I investigated why those were being assigned to class vars on the Assert, I couldn't really figure out the point. Moving them to another file to prevent circular ref worked and all tests passed, so I went with it...
There was a problem hiding this comment.
Happy with that. Talking with Vincent we are fine with the change in public interface as no one should really be using this "feature/work around".
|
The FMX loggers from embarcadero are DUNitX.Loggers.GUIX.pas and DUNitX.Loggers.MobileGUI.pas - Don't worry about them though, they don't compile on XE7 - I guess embarcadero don't care about making things work on older versions of delphi. What versions of delphi do you have. |
| @@ -359,6 +361,23 @@ class procedure Assert.AreEqual(const expected, actual: boolean; const message: | |||
| FailFmt(SUnexpectedErrorStr ,[BoolToStr(expected, true), BoolToStr(actual, true), message], ReturnAddress); | |||
| end; | |||
|
|
|||
| class procedure Assert.AreEqual(const expected, actual: string; const compformat: TDUnitXComparableFormatClass; const ignoreCase: boolean; const message: string); | |||
There was a problem hiding this comment.
The AreEqual seems not to be using TDUnitXComparableFormatClass before the test is failed with the creation of the exception object. If it isn't used in the determination of failing the test, is this adding a purpose to the assert class other than the failure/passing of tests?
There was a problem hiding this comment.
Ok - I figured this would need some explaining. This was the best way that I could come up with for a pluggable formatting identifier. If I used an enum, code would have to be changed to hardcode in new formats (currently I put in XML and CSV). Using a string identifier would be ok, except that then the override signature gets ambiguous. I originally went with an integer, but again, there would have to be a const somewhere to ID it and basically would make the formats supported hardcoded in core files.
I decided instead to create a base class, then use the class as the identifier. The class is not supposed to be instantiated - it is merely a fancy identifier (and strongly-typed one) that allows for flexibility down the road. Currently I ONLY use the classes to ID the format of the data (i.e. XML) so BeyondCompare can use the XML compare instead of straight text. However, later, the classes could actually implement some class procs or vars that customize things, like "compare with or without CRLF".
I know the naming is a bit heavy handed (I believe in being verbose on naming in large projects) but I ultimately think this was the most elegant way to handle identification of formats with the added benefit of giving a place to have format specific config later on.
There was a problem hiding this comment.
Thanks for the detailed explanation. Happy with this, I don't have any better options for adding data definition to the assert tests.
|
Just committed the project files with the old GUI file removed. Also committed changes based on feedback from @staticcat. I only have XE and 10 Seattle currently installed. I don't do component work, so I don't have any of the others installed. My main app dev is still in XE, but we are moving certain things to 10 (and I guess 10.1 now) so thus I have one of the oldest and one of the newest :) |
| if ignoreCase then | ||
| begin | ||
| if not SameText(expected,actual) then | ||
| FailStrCompare(expected, actual, compformat, message, ReturnAddress); |
There was a problem hiding this comment.
The caller can pass the test even if their data does not match the supplied formatter requirements. E.g. "Something that isn't xml".
There was a problem hiding this comment.
Yes, that is correct. The intention was NOT to add more granular capabilities to the Assert, but to be able to control the diff on the GUI side. In other words, the format classes (as of now, and as I originally have intended) were to ONLY be used for helping you diff the failure - not change the comparison. In my mind, your test should return the EXACT result you are expecting when it comes to strings (so if expecting CRLF, it should have CRLF), but when it comes to diffing said result, you may want the niceness of the compare tool's formatting capability. We have a lot of XML that gets generated that can't have CRLF on it. Diffing that without BeyondCompare formatting it is a nightmare (one long line), thus why I even allow specifying a format in the first place. Yes, you can manually select XML in BC, but when you are working bugs quickly, the number of clicks really starts to be prohibitive. Perhaps, down the road, the Assert could take some rules into account, but that wasn't my intention at this stage. It is just a way for the GUI to know what the format is SUPPOSED to be and how best to diff it.
|
So I have reviewed everything. The change looks great, fantastic in-fact. Apologies for the spam, just updating things as I saw them in the change view. Great to see the amount of effort that has gone into this change. Having the GUI logger and view will be a widely used addition to DUnitX. Heaps of people say they don't use it as it doesn't have a GUI viewer. Only two things we will have to watch out for, and possibly improve in future:
So my thumbs up, and props to @rmcginty for the submission. 👍 |
|
Well thank you - I have more ideas down the road, so I'm excited to be able to contribute them to the bigger picture :) DUnitX's core is awesome and I am happy that I am able to use it along with the unique things I had in my proprietary test suite! |
|
Just a thought for the future, make the diff tool configurable (per user), not everyone uses BC (we do though so I'm ok with this for now). |
|
Yes, I was thinking about that, but decided to hard code for now and make sure you guys were cool with the changes I had already made. A pluggable model would be perfect, and you could also have different diffs based on datatype (one for XML, one for CSV, etc). Also figure we can do binary diff as well, but that isn't something I do very often, thus I've never experimented with that. The other issue is that it "pulls the thread" - if I have multiple supported diff tools, then I'm probably going need to actually store some config somewhere (preferred diff for X format, options, etc) so that is a whole other chunk to worry about while keeping it lightweight, Delphi independent, and compatible with all platforms. I definitely have ideas for it though. |
|
I'll leave this open for a day or two so others can chime in, but I'm happy to merge. Thanks for the contribution. |
|
Sounds good - please note that I have NOT made any changes to the expert or anything like that. I plan to, but if someone else gets to it before me, my feelings won't be hurt :) Right now, someone that wants to use it pretty much has to look at the examples and do it manually. |
| @@ -48,21 +49,22 @@ interface | |||
| Assert = class | |||
| private | |||
| class var fOnAssert: TProc; | |||
| class var fTestFailure: ExceptClass; | |||
There was a problem hiding this comment.
D'oh - did you understand what these fields were for? This was to not couple the assert class with the framework (yes, I could use this in a DUnit project!) but now you went back how it was before by hardcoding the exceptions.
There was a problem hiding this comment.
I didn't realize that is why this was done. There weren't any comments explaining that, so I thought it was just something left over from the past, or a way around a circular reference. I had to eliminate the "generic" exception because I couldn't get away with just message - I needed to add custom data to the exception. I could revert the changes and make some new exception classes, but dumb down to the lowest common denominator if any class var is nil (for instance, if fTestFailureStrCompare = nil, raise fTestFailure) and I think that would satisfy the requirement.
There was a problem hiding this comment.
In that case you might want to add your new feature either as helper for the Assert class or via Event that can produce the proper exception. In any case try to keep Assert as simple as possible without cluttering it with special framework related cases (personally I see the diffing logic as special case).
I for example also had the problem of comparing long strings (xml) via unit test and the DUnit error message was not very helpful telling me that differs from so I implemented my own CheckEqualsString method that only shows a snippet where the strings differ and the position where that is)
There was a problem hiding this comment.
If Assert is going to need to stay framework-agnostic, then yes my changes will have to be done differently. I can't really go the helper route because you can only have one helper per class and the helper would also have to be included in the project or a uses clause (could confuse people), and that would limit anyone else from adding a helper later. One of my goals was to integrate this directly in Assert. While you consider the diffing logic a special case, it is not only for diffing, that is just how the GUI logger uses it. It could be used by automated systems to save the data out and add as artifacts in the CI so people could review the output in a more digestible format. The problem is the data that you are asserting is locked in a string error message which makes it hard to act on if desired.
While I knew Assert was supposed to remain as self-contained as possible (which it still is), I did not know it was supposed to remain framework independent (which it now is not because of the exceptions). I will see about getting the latter back - I am not exactly sure the best way to do that but will explore some options.
|
My only concern was the hard coding of the exception classes that @sglienke addressed. I was able to create example projects for XE6 and Berlin and verify everything still works. Then used @rmcginty code with our own internal tests and everything stayed functioning as expected for us. Had to modify some of our custom asserts, but it was not too much work. |
|
I'm chewing on the hard coding of the exception classes. Going to experiment a bit after hours and see if I can find an elegant solution... |
|
Ok - so I can revert the changes to DUnitX.Assert to put it back almost exactly the way it was BUT it would mean creating a new Assert file specific to DUnitX that derives from the existing Assert, then pointing the test framework to the new Assert that contains my functionality. I have modified my working files to test this, and it does work. My only concern is what to call the new file AND do we really want to do it this way? Honestly, imo, if the old DUnitX.Assert was to remain framework agnostic, it should have been called something else (like Generic.Assert), but obviously renaming the file would cause breaks. For the test, I simply called the new file DUnitX.Assert.Ex, but that was for lack of a better idea. To give you an idea of what I mean, in TestFramework, it would look like this:
And the new Ex file has my changes declares another Assert like:
The only change to the old assert file is me moving a couple of things (like DoAssert) from private to protected. So, I need some guidance. Leave the way it is now in the PR or revert Assert and go with a new file for the new comparison stuff and, if so, what file name do you think fits best? |
|
Could this done through a helper class for assert? ComparableAssert = helper for Assert This could be defined inside DUnitX.TestFramework.Comparable or such unit. To fix the coupling in TDUnitXTestError, change the members added there to a simple FExceptionData : TObject. This data would come from a abstract function off the base exception class EAbort. The FIsComparable, FExpected, and FActual might be candidates for keeping in the base TDUnitXTestError. This allow the member variables for the exception classes to stay in the Assert class. It would then allow anyone to augment Assert and add new functionality and data pass through. |
|
That does work - my only concern with helpers is that you only get one per class (if I remember correctly) so we might want to call it DUnitX.Assert.Helper so people know where to look. It could be used to augment beyond just comparison functionality. I've not made the changes for the TestError data yet, but will here soon. I think the helper approach is palatable enough without any negative side effects, so unless there is an objection to the .Helper name, I will make the changes and update the PR code. |
|
Hmmm. Your right about one helper at a time being applied. You can define as many as you like though. Last scope wins. Helpers might not be the way. Maybe just add another class called ComparableAssert and it can be used in conjunction with Assert. Better yet seeing as the tests are string based StringAssert. I would suggest not sub-classing Assert as this would confuses people on feature sets. I also wouldn't suggest moving all string tests to the new class for backward compatibility reasons. This could be done in a major release possibly. How does that sound? It would mean only an extra uses include, and switching between StringAssert and Assert for testing. |
…est, added actual/expected copy to clipboard actions to GUI
|
Well, one of the things I really liked about DUnitX was the single Assert. It is like CodeSite and Send. You don't have to dig to find the procedure you wanted. I really would like to stick to that. I know you said not to subclass Assert, but the solution works well and I don't think it adds much confusion as the original Assert file really needed a large comment mentioning that it should be kept isolated (which I threw in there). I have updated the PR with the subclassed solution for people to comment on and see the whole thing working. Personally, of all the options CURRENTLY, I like this the most and it should satisfy all requirements on the table. I, admittedly, don't know what ideas you guys have for the future though, so if this doesn't fit in with that vision, either feel free to change or let me know what you'd like me to change. |
…r integer that seemed missing
|
You guys want me to do anything else on this? I have more changes, but they extend outside of the GUI changes, so I figure those would be best reviewed separately. I do plan to implement the generic object to hold extended properties on the error object, but just haven't gotten around to doing so yet. I've been using TestInsight, console, GUI, and XML output (via Continua) for a couple of weeks now and have had zero unintended side effects that I can find. If you don't have a problem with me continuing to add to this PR, I can, but again I'm assuming it would be easier to review if it was broken into multiple PRs over time. |
|
Sorry, just been extremely busy so haven't had much time to review this yet. We will definitely merge in the next week or two. |
|
No problem. I just wanted to make sure you weren't waiting on me. I will probably still push some incremental changes related to GUI, unit tests, etc (easy things to review) in the meantime but save my bigger changes (changes to the expert in particular) for after this PR is merged. Thanks for the reply! |
By default, string comparisons were ignoring case which is opposite from the way I ran my tests. In order to not have to change my tests, and not break backward compatibility, I added a property that can be set per project to identify if string compares are case sensitive by default.
…ld fixtures, adding GUI to test proj
…llName duplicates
|
Ok, now had some time to look at this, looks ok to merge, however we should probably update the wizard to generate code to use the gui, much like we do for Testinsight etc. If there are no objections or further comments I will merge this in the next day or two. |
|
I have a ton of other changes (FileAssert, temporary file management for file tests, etc) that I'm working on right now that have not been pushed. I will be addressing the wizard and experts in the next two weeks, if not sooner, but I think it would be good to go ahead and merge what is there so it will be a bit easier to review the expert changes and new additions separately. I just wanted you to know that I am planning on updating the expert sooner rather than later. I also will throw together a video of tips for using the GUI to rapidly debug once the merge happens and people have access to it. Lastly, I'll work on the docs in the wiki as time permits. |
|
No further comments so merging. I did pull it down to play with today, IDE doing annoying things with the uses clause on the form so will have to find another way to deal with namespaces. Also, the form doesn't respond while the tests are running, because it's all running in the main thread. I have some ideas on how to fix so will investigate options. Moving the runner to another thread would be problematic, but it's something that might be a problem in the future as we have plans to enable parallel running of fixtures at some stage (hence the threadId on the logger interface methods). |
|
Yes, both things I'm aware of. Updating the form in the newer IDE was a major pain requiring me to edit it outside to get it right. I tried everything I could think of, but the form designer autogen code apparently does NOT take into account conditionals at all, making it very problematic. It would be nice if there was some directive you could put up top to disable it for cases like this (I mean, cmon, we can figure out why the compiling fails on a TEdit if the uses clause is inaccurate), but I don't think something like that exists. Delphi XE didn't have a problem with it, but that may be because the ELSE contains the non-namespaced uses. I do all my editing on that form in XE to ensure the properties are all forward compatible with the newer versions. As for the UI not updating, it was not something I wanted to dig into just yet but ultimately a callback and/or multi-threading would definitely add to it. There are a few times where a test takes much longer than I expect and it would be nice to know which it is and maybe even have the ability to cancel the run. I have more changes coming soon - some UI enhancements to the GUI and a few bug fixes, but trying to get this FileAssert and some built-in normalization tools (for instance to find all dates via regex and replace them when you have data that is timestamped etc) finalized for review. I even went so far as to at some conditionals to allow using the TRegExpr component if installed in Delphi versions that didn't support RegEx already (and extended that to the Assert.IsMatch too). I've also made to sure to keep the tests all up-to-date to keep coverage solid on everything new. |
Most of the changes here are substantive, but there are a couple of files that I cleaned whitespace. I've tested DUnitX unit tests and GUI in Delphi XE and 10 Seattle, but those are the only two I currently have installed. If anyone feels there are issues in other versions, please let me know.