pytest: Fix typo and add logging #54027
Conversation
zephyrbot
requested review from
PerMac,
aaronemassey,
asemjonovs,
galak,
gopiotr,
hakehuang,
jeremybettis and
yperess
|
This is meant to be rebased and merged as two separate commits. Please let me know if I didn't format the PR properly for that. |
There was a problem hiding this comment.
I'm not a particular owner of this file, but I'd be hesitant to add a class here for a single example that only shows string formatting.
There was a problem hiding this comment.
I'm not seeing the benefit of making a class from this, can be done inline on line 218
There was a problem hiding this comment.
I'm not seeing the benefit of making a class from this, can be done inline on line 218
|
Doing it inline forces all of the string formatting to be done even if debug logs are turned off. Putting it in an object delays the formatting and skips it all together if the log is not rendered. In general, I prefer not to do expensive operations in debug logs for performance reasons, but this is not a performance-critical codepath, so maybe it would be okay. Please let me know your preference. |
That's a good point! Since we're not just using the string formatting provided by the logger we don't get the benefit of lazy formatting from the logger. However:
I'd rather have a nominal increase in compute cost, especially if not performance-critical, instead of adding an additional type. If this changes we can always add the class or use lazy-string but I don't foresee this to happen. |
|
Updated. Is this ready to rebase+merge? |
Fine with me, but you should get a review from a proper maintainer of twister. I'd recommend the pr help channel in the discord. |
|
@nashif is listed as the Twister maintainer. |
dreiss
force-pushed
the
pytest-1
branch
2 times, most recently
from
eaaf898 to
f78e460
Compare
|
Any ideas about this failure? I locally ran |
The option is "cmdopt" Signed-off-by: David Reiss <[email protected]>
This makes it easy to run the command manually, possibly with edits to change the test behavior. Signed-off-by: David Reiss <[email protected]>
|
Looks like it was fixed by rebasing. Can this be merged? |
We are currently in hard freeze for 3.3 and only blocker bug fixes and documentation changes can be merged now. The merge window will open next week. |
6 participants
No description provided.