This adds unit tests to the ScopedPrinter class.
Depends on D114223
Differential D114684
Add ScopedPrinter unit tests Jaysonyan on Nov 28 2021, 7:11 PM. Authored by
Details This adds unit tests to the ScopedPrinter class. Depends on D114223
Diff Detail Event Timeline
Comment Actions Address patch comments.
Comment Actions Looks like you're still missing tests for startLine and getOStream, which should be simple, but for completeness should be added too.
Comment Actions Add missing tests and update existing ones.
Comment Actions Nearly there, just one actual comment.
Comment Actions Should unit tests be added for to_hexString(), to_string() and enumToString() (from D114840)? Although they are not part of the ScopedPrinter class but just in ScopedPrinter.h. Comment Actions Yes, they should be added, but I'd be inclined to say that they should be in a separate patch, allowing us to focus this on the ScopedPrinter class specifically. @Jaysonyan - I spotted a unit test failure in the pre-merge checks above. You should take a look and address it. I think it's a classic case of a compiler treating a uint8_t/int8_t as char variants, and printing them as such. At a guess, this is a bug in the main code that the test is highlighting, rather than a bug in your change though. Is this bug present with the virtualization patch? In theory, this patch should be standalone, so I think a separate precursor small bug fix would be beneficial, so that this unit test patch can be submitted immediately. Aside from that, looks good.
Comment Actions
I can add these in a separate patch then.
This was working locally and I believe that this might because I didn't mention that this diff depends on the virtualization diff. I've added that to the description and hopefully the failing test will fix itself. That being said, I think this indicates that the virtualization patch (D114223) includes a functional change (calling printList with uint_8/int8_t). Although I believe this would be closer to fixing an unrealized bug. Comment Actions Agreed that this was probably a bug-fix. Might be worth highlighting this in a comment in the patch description.
|
I don't know what "Fcn" stands for. At a guess, it's either "function" or "functor" or similar. I'd suggest "Func" maybe? Similar throughout.