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

Include location footer in logs for AssertionErrors #128

Conversation

jayeshathila
Copy link
Contributor

@jayeshathila jayeshathila commented Feb 12, 2020

Changes:

1. Adding Location footer in AssertionErrors.

Concerns:

1. Didn't add tests for the change as I can't find other tests for the Locations.
2. Approach doesn't seem clean to me, not sure if how we can improve it.

issue #126

@jayeshathila jayeshathila marked this pull request as ready for review February 14, 2020 11:51
@darrenburns
Copy link
Owner

I wonder if there's a way we could have the location always be the line number that the test failed on, just to keep things consistent throughout? If so, I also think we should print out the surrounding context of where the failure occurred, as per the "Failure" header shown in the image below.

Screenshot 2020-02-04 at 22 26 52

@blueyed
Copy link
Contributor

blueyed commented Feb 21, 2020

The location (file + lnum) of the test failure is very useful, since it allows to quickly go there in an editor.
Keep in mind though that it might fail before or after the actual test however (fixture setup/teardown etc). In that case it should be the location of the "crash" probably.

@darrenburns
Copy link
Owner

Very good point @blueyed.

There's two relevant lines: the location of the failing test definition, and the location the failure/error actually occurs. I wonder what the least confusing way to communicate those to the user is? Any thoughts @jayeshathila? I'd be open to changing the existing output if we can think of something clearer.

@blueyed
Copy link
Contributor

blueyed commented Feb 23, 2020

With pytest I am using nodeid::test_file.py:42 by default, but expand it to nodeid::test_file.py:42 (some/other/file.py:123) if the crash happened somewhere else. This provides two locations then: one to easily go to the test's code that triggered it, and another for going to the crash itself (which might be a lib or a non-hidden test helper).

@jayeshathila
Copy link
Contributor Author

Very good point @blueyed.

There's two relevant lines: the location of the failing test definition, and the location the failure/error actually occurs. I wonder what the least confusing way to communicate those to the user is? Any thoughts @jayeshathila? I'd be open to changing the existing output if we can think of something clearer.

Ya, I am myself aligned with that we should print the line_num where failure actually occur. Will see what can be done to print the line_num. Will update my finding here.

@jayeshathila jayeshathila deleted the origin/include_location_for_assertions branch February 25, 2020 18:43
@jayeshathila jayeshathila restored the origin/include_location_for_assertions branch February 25, 2020 18:46
@jayeshathila jayeshathila reopened this Feb 25, 2020
@jayeshathila jayeshathila force-pushed the origin/include_location_for_assertions branch from dfef175 to a58489a Compare February 25, 2020 18:59
@jayeshathila
Copy link
Contributor Author

@darrenburns @blueyed I have added line_number explicitly (using traceback) in case of AssertionError while creating TestResult here. Any thoughts on the approach ?

ward/testing.py Outdated
@@ -217,6 +219,12 @@ def get_result(self, outcome, exception=None):
if outcome in (TestOutcome.PASS, TestOutcome.SKIP):
result = TestResult(self, outcome)
else:
if isinstance(exception, AssertionError):
exc_traceback = sys.exc_info()[-1]
Copy link
Owner

Choose a reason for hiding this comment

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

My understanding is that sys.exc_info only works in the context of except blocks?

I think you can get the traceback from the exception using exc_traceback = exception.__traceback__.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that sys.exc_info only works in the context of except blocks?

I think you can get the traceback from the exception using exc_traceback = exception.__traceback__.

Ah. Agree get_result being public method I shouldn't expect it to be in context of except. Will change this to use exception.__traceback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darrenburns, Updated with exception.__traceback__.

@jayeshathila jayeshathila force-pushed the origin/include_location_for_assertions branch from a58489a to 6c4fa42 Compare February 27, 2020 14:04
@darrenburns
Copy link
Owner

Looks good, thanks @jayeshathila. Will take a closer look asap!

@darrenburns darrenburns merged commit 2450038 into darrenburns:master Mar 7, 2020
@jayeshathila jayeshathila deleted the origin/include_location_for_assertions branch March 7, 2020 14:43
@darrenburns
Copy link
Owner

This change has been included in Ward 0.39.0b0 - thanks again @jayeshathila :)

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

3 participants