Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unittest: Log failure tracebacks at test end. #8

Closed
wants to merge 1 commit into from

Conversation

andrewleech
Copy link
Contributor

Store traceback details for each test failure and log to console at the end of the test run.

This prints out any exceptions, one after another separated by a line of ---'s more similar to the cpython unittest failure printouts, removing the need for the suggested un-commenting of an exception raise line to find out information about the failure.


def wasSuccessful(self):
return self.errorsNum == 0 and self.failuresNum == 0

if hasattr(sys, 'print_exception'):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you see around this file, doublesquotes are used. That's my default style for strings. So, I would ask you do update your changes (more cases elsewhere).

As for this chunk of code, well, I remember some people wanted to use unittest on devices. I guess, this can be seen as courtesy to them (instead of depending on traceback module right away). But note that feature you add, accumulating large objects in memory, goes against such usage in the first place. Nothing needs changing here, just food for thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point noted on string style, thanks. I'll clean up pre-existing inconsistencies while I'm at it.

With the print_exception stuff, yes I'm using unittest on both unix and stm32; unix for development and CI, stm32 for product proof / hardware in the loop testing.
This allows us to run the exact same tests on both CI and on hardware via pyboard.py

I'm probably up against the limit of my knowledge of the vm & gc, would you mind highlighting the biggest costs of this implementation of a print_exception function / handle? I do want to make my work as efficient as practical.
Other than the parsing and compiling costs of more code text, is it the lamba usage adding the extra layer of abstraction? Does having it at global scope make it worse? Am I missing something bigger?

That block I did copy from some other library, looking again it really does not need to be standalone and the functionality should just be moved into def capture_exc(e).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll clean up pre-existing inconsistencies while I'm at it.

No, please ;-). Just use the style the majority of file uses. (And you now know what should be majority ;-) ).

As separate work/PRs, might make such cleanups of course, but there're so many ways to spend time on micropython-lib in more useful ways, so I'd discourage spend it on strings either.

P.S. There're tools which can do that automatically.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for other questions, I'm not sure where they come from. For some other module - sure, may makes sense to think about all those things (and great that you do). But for "unittest"? Nah.

"But note that feature you add, accumulating large objects in memory" means just what it says:

You do:

exceptions.append(...)

So, if each of 1000 tests throws exception, you'll have a list with 1000 exception tracebacks. That's fully ok for this kind of module from my PoV.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I completely follow you now. Yes it's a risky pattern to follow, unbound length arrays growing like that.

Copy link
Owner

@pfalcon pfalcon Nov 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not "risky", rather "desktoppy" and not "embeddy". As unittest isn't really intended for embedded usage, that's ok. But knowing that some people (like yourself) use it on embedded is helpful. (When needed, we can split it as uunittest, though I'd hope that for embedded usage, someone would come up with purposedly designed module at all.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even on a desktop it's often bad practice, but yeah it's usually a deal breaker on embedded. My background is in electronics and small embedded work, but I find myself getting very lazy with python which is a dangerous mentality to get into (even though our stm32 board has 32MB of sdram).

I've considered splitting modules myself for smaller embedded targeting version, though I'd want some significant differences between it and a full unix version to be worth maintaining two copies.
I'm working on a pathlib port from the cpython impl in the background at the moment though and that's a definite candidate for two versions.... the initially mostly working copy is still a 42KB python file, that's not going on many embedded platforms as is for sure.

@@ -214,11 +232,15 @@ def test_cases(m):
if isinstance(c, object) and isinstance(c, type) and issubclass(c, TestCase):
yield c

m = __import__(module)
m = __import__(module) if isinstance(module, str) else module
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I smell this is not related to "log failure tracebacks at the end". Ok for this time, but next time I'll ask to make this a separate commit ;-).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about this change sneaking in as well. I will certainly split it into a separate commit at least.

@andrewleech
Copy link
Contributor Author

Ok so it's very easy to test my own changes.

Testing my initial implementation (bf2a008) with an Exception added to one of the test_unitest tests reports 37600 from gc.mem_alloc() after the test suite runs.

The same test with just pushed commits (5e10ff5) reports 37472 to run the same suite.

@pfalcon
Copy link
Owner

pfalcon commented Nov 2, 2018

Ok, I've merged the "unittest: Allow passing module name or instance into unittest.main()" commit.

Unfortunately, for the other commit, you would need to undo unrelated cosmetic changes. Sorry for this back-and-forth again. It's one the most baseline rules of both core MicroPython and related projects: "No unrelated changes in one commit, and cosmetic fixes are always unrelated." I take for granted that everyone knows it ;-). Contrib guidelines here refer to MicroPython's where it's spelled out. I also try to update guidelines here with cases we encounter lately. Please feel to review and/or clarify some points if not clear.

More about cosmetics: if done, they should be done globally. And that's a lot of work, which better be spent on something else for the foreseeable time.

So again, local consistency is what rules. If line above the one you patch has "", please don't add below it ''. That's about it. Within a single func, better to use the same quotes. Beyond that - well, inconsistencies happen. You by now should know how - I initially wrote it with doubles, then someone contributed code with singles, and wasn't strict enough with that matter.

Store traceback details for each test failure and log to console at the end of the test.
@andrewleech
Copy link
Contributor Author

Yes, you're right, of course I know about not mixing cleanups with code. You shouldn't have to tell me, yet you did. Don't know what I was thinking, thanks for pulling me up on it.

Speaking of tools though, have you heard of Black? https://github.com/ambv/black
I've wanted to role it out on my projects for a while, just haven't been able to bring myself to accept the look of dedented function args, even if I accept the motivation.

@pfalcon
Copy link
Owner

pfalcon commented Nov 3, 2018

Thanks. Pushed/released. But I've edited commit message to mention that the change is made to match CPython's behavior (and not otherwise arbitrary).

Btw, feel free to mentioned explicitly when you've done changes and it should be ready for re-review, because otherwise I don't really have means to know that ;-).

@pfalcon pfalcon closed this Nov 3, 2018
@pfalcon
Copy link
Owner

pfalcon commented Nov 3, 2018

Speaking of tools though, have you heard of Black?

I heard, yeah. Now I even looked during the tea break. It uses "right" string quotes, good. (Btw, as a C programmer, it shouldn't surprise you why doublequotes is the default delim for strings ;-) ).

I usually don't use such stuff in "my own free time". The whole idea of Python is that its syntax naturally enforces standard codestyle to the large degree. There're lot of small details of course, but usually there always something more important to do than to bother automating them. As a random example, I'm trying to pool time to look into more detail/comment on micropython/micropython#4195 .

That said, if you have any ideas about codestyle tools, etc., feel free to share them, just don't be surprised if conversation goes slow/backlogged ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants