-
Notifications
You must be signed in to change notification settings - Fork 241
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
Post process html to include teardown in log #271
Conversation
Wow. Those unit tests really helped me fix a lot of things i had broken :P. Honestly don't know if this is good enough to merge... it seems kind of sketchy... but per the tests seems to work. :) |
Thanks for this! @csm10495 I need some time to fully understand the problem and the fix before I can review this with confidence. Feel free to ping me if you haven't heard anything from me in a week or so. |
Sure thing @BeyondEvil . The big thing that I want to fix is to get the teardown event in the html log: for example:
In doing that, i try to hold back from processing the reports, until we get to the |
@BeyondEvil reminder to check this out. Thanks. |
Reminder to check this out @BeyondEvil |
@csm10495 @BeyondEvil I'd like to help with that functionality. Do you need anything extra? Interesting fact. I'm able to see |
Hi folks, any update about this one? we're waiting for it as well :). |
Hi @csm10495 - i have tried your code and it works good while collecting logs from teardown of a fixture and showing them in the html report, |
Hey @dina809 , it's been a while now since I've looked deep at this. What error are you seeing? It works if I do this:
|
Bump. Did someone get a chance to review this? |
Any release date for this? |
I'm hoping @ssbarnea or @gnikonorov can find some time to review this? |
I can take a look at this tonight, @BeyondEvil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, thank you @csm10495 and sorry for the very long delay here! I just had a few questions.
I'll be staying on top of this PR. If you haven't heard from me in a day or so, @ mention me so I see the notification
@gnikonorov, I made some fixes and added a comment. Tests pass feel free to merge if its good. Also, the linting is weird since black wants something different on gh-actions vs travis... so we need to pick one or the other :P. Though I will say that I wrote this a long time ago for a previous task, so I don't quite have the right mindset to fully test it again. Hoping the new unit tests (that i wrote back then) are good enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the prompt update, @csm10495! I had one more batch of comments.
Once those are addressed, I'd be happy to merge assuming @ssbarnea can take a look as well and OK this.
Also, can you add a changelog entry for this under 2.1.2 (unreleased)
in https://github.com/pytest-dev/pytest-html/blob/master/CHANGES.rst
@gnikonorov , changes made. Hoping we're all good :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnikonorov , changes made. Hoping we're all good :)
We are @csm10495, thank you for the prompt responses. I'll wait for @ssbarnea to take a look, but this LGTM to me
Please rebase. I do support the change, is very useful to have teardowns with results. |
3e6d07e
to
911fb59
Compare
Thanks @ssbarnea and @gnikonorov. Squashed things down. Should be clean for merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'll leave the merge to @ssbarnea so he can take a look at the final version too
I believe this would resolve #131 ... however it isn't really optimal, I didn't want to dig into the html logic, so instead just moved it to be post processed and tried to coerce the test result into what the existing code seemed to want.