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

Testing API doesn't yield useful results #4302

Closed
yash-nisar opened this Issue Jun 1, 2017 · 6 comments

Comments

4 participants
@yash-nisar
Member

yash-nisar commented Jun 1, 2017

The current testing API doesn’t provide useful results as a traceback if a given test fails. It becomes a strenuous and time consuming job to follow the traceback searching for errors.

Eg. For the check_results(...) method, http://paste.ubuntu.com/24269550/ is the output that I received when I had a minor difference of line=5 instead of line=6 in the given code snippet :

[Result.from_values('ElmLintBear',
                     message=result_message_bad_function,
                     file=fname,
                     line=5,
                     severity=RESULT_SEVERITY.NORMAL)]

The given output below appear to be strange as both the Result objects contain the same number of characters (632) due to which testing transforms to a challenging task. Hit and trial methods or the usage of a debugger are the only possible ways to overcome these errors.

E           AssertionError: Lists differ: [<Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[632 chars]710>] != [<Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[632 chars]400>]
E           
E           First differing element 0:
E           <Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[631 chars]0710>
E           <Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[631 chars]0400>
E           
E           Diff is 2804 characters long. Set self.maxDiff to None to see it. : The local bear 'ElmLintBear' doesn't yield the right results. Or the order may be wrong.
E           Running bear ElmLintBear...
E           Running 'elm-format /tmp/tmpzu0n4474 --yes'

difficulty/medium
area/tests
type/test

@jayvdb

This comment has been minimized.

Show comment
Hide comment
@jayvdb

jayvdb Jun 3, 2017

Member

@yash-nisar , but if you set self.maxDiff = None, the output becomes much more usable, but the pytest approach of showing all of the backtrace results in a horrible maze of information.

We actually have a few self.maxDiff = None littered around the tests.

Member

jayvdb commented Jun 3, 2017

@yash-nisar , but if you set self.maxDiff = None, the output becomes much more usable, but the pytest approach of showing all of the backtrace results in a horrible maze of information.

We actually have a few self.maxDiff = None littered around the tests.

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 3, 2017

Member

It's still quite some output, so we make it a bit more precise 👍

Member

Makman2 commented Jun 3, 2017

It's still quite some output, so we make it a bit more precise 👍

@jayvdb

This comment has been minimized.

Show comment
Hide comment
@jayvdb

jayvdb Jun 4, 2017

Member

The design at https://yash-nisar.github.io/blog/testing-api-coala/ doesnt indicate how you will improve on the pytest output (assuming self.maxDiff is disabled), which is mostly ugly and unusable because of the depth of the stack, and volume of information with self.maxDiff is disabled, and I dont see that being addressed in the proposal.

Member

jayvdb commented Jun 4, 2017

The design at https://yash-nisar.github.io/blog/testing-api-coala/ doesnt indicate how you will improve on the pytest output (assuming self.maxDiff is disabled), which is mostly ugly and unusable because of the depth of the stack, and volume of information with self.maxDiff is disabled, and I dont see that being addressed in the proposal.

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 4, 2017

Member

I think addressing the output stack is quite some problem that is beyond gsoc. The pytest guys didn't do ugly outputs on purpose, I assume they already tried to make it as readable as possible without loosing information. You are right, there's still much space for improvement, but that's another topic.

We are concentrating on results from bears. This is bad:

E           AssertionError: Lists differ: [<Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[632 chars]710>] != [<Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[632 chars]400>]
E           
E           First differing element 0:
E           <Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[631 chars]0710>
E           <Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[631 chars]0400>
E           
E           Diff is 2804 characters long. Set self.maxDiff to None to see it. : The local bear 'ElmLintBear' doesn't yield the right results. Or the order may be wrong.
E           Running bear ElmLintBear...
E           Running 'elm-format /tmp/tmpzu0n4474 --yes'

self.maxDiff = None doesn't make it really better and is still a pain. That's why we make a more precise assertion message, that will look like

E   AssertionError: 'StylintBear' != 'StylintBea'
E   - StylintBear
E   ?           -
E   + StylintBea
E    : origin mismatch

Ideally all objects that use generate_eq or generate_ordering will get this improved message automatically through all tests. Currently we aim only for bear tests specifically, but that would be the super goal.

Member

Makman2 commented Jun 4, 2017

I think addressing the output stack is quite some problem that is beyond gsoc. The pytest guys didn't do ugly outputs on purpose, I assume they already tried to make it as readable as possible without loosing information. You are right, there's still much space for improvement, but that's another topic.

We are concentrating on results from bears. This is bad:

E           AssertionError: Lists differ: [<Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[632 chars]710>] != [<Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[632 chars]400>]
E           
E           First differing element 0:
E           <Result object(id=0xce0383d576a84f34848d66d0ddc87479, origin=[631 chars]0710>
E           <Result object(id=0x7e58527926a345f8a2bb36305c58f9f6, origin=[631 chars]0400>
E           
E           Diff is 2804 characters long. Set self.maxDiff to None to see it. : The local bear 'ElmLintBear' doesn't yield the right results. Or the order may be wrong.
E           Running bear ElmLintBear...
E           Running 'elm-format /tmp/tmpzu0n4474 --yes'

self.maxDiff = None doesn't make it really better and is still a pain. That's why we make a more precise assertion message, that will look like

E   AssertionError: 'StylintBear' != 'StylintBea'
E   - StylintBear
E   ?           -
E   + StylintBea
E    : origin mismatch

Ideally all objects that use generate_eq or generate_ordering will get this improved message automatically through all tests. Currently we aim only for bear tests specifically, but that would be the super goal.

@jayvdb

This comment has been minimized.

Show comment
Hide comment
@jayvdb

jayvdb Jun 4, 2017

Member

This is bad:
...

Yes, that is bad, but that is without self.maxDiff = None, which is irrelevant as that is not a good baseline for comparisons. We can force self.maxDiff = None on every test with one line of code.

I would very much like to see the output of the same assertion with self.maxDiff = None, as that should be the baseline that we compare enhancements to.

I dont see anything in the design to show how we will get the errors to look like

AssertionError: 'StylintBear' != 'StylintBea'
...

The design indicates this will happen without any change in the bear tests, so it will happen here in the coala test helpers.

One way to do that is to iterate over each attribute and do an assert. Is that the plan?
Then how to handle when there are multiple attributes which are different?
The risk is that the inserted assertions may show too little information, focusing on only a single different attribute when there are larger differences also occurring.

Another consideration (which I havent looked into at all), is whether a pytest extension might also provide better test error information. There is a very large ecosystem of pytest extensions.

Member

jayvdb commented Jun 4, 2017

This is bad:
...

Yes, that is bad, but that is without self.maxDiff = None, which is irrelevant as that is not a good baseline for comparisons. We can force self.maxDiff = None on every test with one line of code.

I would very much like to see the output of the same assertion with self.maxDiff = None, as that should be the baseline that we compare enhancements to.

I dont see anything in the design to show how we will get the errors to look like

AssertionError: 'StylintBear' != 'StylintBea'
...

The design indicates this will happen without any change in the bear tests, so it will happen here in the coala test helpers.

One way to do that is to iterate over each attribute and do an assert. Is that the plan?
Then how to handle when there are multiple attributes which are different?
The risk is that the inserted assertions may show too little information, focusing on only a single different attribute when there are larger differences also occurring.

Another consideration (which I havent looked into at all), is whether a pytest extension might also provide better test error information. There is a very large ecosystem of pytest extensions.

@Makman2

This comment has been minimized.

Show comment
Hide comment
@Makman2

Makman2 Jun 4, 2017

Member

See https://yash-nisar.github.io/blog/testing-api-coala/ on how to achieve this.
Basically we want to have better assertions for objects using generate_eq and generate_ordering. As Result is using generate_ordering, it automatically affects bear tests. But all other tests also benefit from a nicer to read message (if we are able to expand this concept to all test).

self.maxDiff = None is nearly never a solution.
Example: https://pastebin.com/Avziwuun
Find the failure ;)
Spoiler: I introduced a typo in one of the messages (vulnerabilities) inside the BanditBearTest. Just removing an l (it's an L ;D) makes the diff unreadable, even having the full diff.

Member

Makman2 commented Jun 4, 2017

See https://yash-nisar.github.io/blog/testing-api-coala/ on how to achieve this.
Basically we want to have better assertions for objects using generate_eq and generate_ordering. As Result is using generate_ordering, it automatically affects bear tests. But all other tests also benefit from a nicer to read message (if we are able to expand this concept to all test).

self.maxDiff = None is nearly never a solution.
Example: https://pastebin.com/Avziwuun
Find the failure ;)
Spoiler: I introduced a typo in one of the messages (vulnerabilities) inside the BanditBearTest. Just removing an l (it's an L ;D) makes the diff unreadable, even having the full diff.

yash-nisar added a commit to yash-nisar/coala that referenced this issue Jun 23, 2017

LocalBearTestHelper: Add ``assertObjectsEqual``
Add ``assertObjectsEqual(..)`` method that compares individual
fields of the Result object and yields better, easy to understand
messages in case of an attribute mismatch.

Closes coala#4302

yash-nisar added a commit to yash-nisar/coala that referenced this issue Jun 24, 2017

LocalBearTestHelper: Add ``assertObjectsEqual``
Add ``assertObjectsEqual(..)`` method that compares individual
fields of the Result object and yields better, easy to understand
messages in case of an attribute mismatch.

Closes coala#4302

yash-nisar added a commit to yash-nisar/coala that referenced this issue Jun 25, 2017

LocalBearTestHelper: Add ``assertObjectsEqual``
Add ``assertObjectsEqual(..)`` method that compares individual
fields of the Result object and yields better, easy to understand
messages in case of an attribute mismatch.

Closes coala#4302

@Makman2 Makman2 removed the type/test label Nov 13, 2017

yash-nisar added a commit to yash-nisar/coala that referenced this issue Feb 2, 2018

LocalBearTestHelper: Add ``assertObjectsEqual``
Add ``assertObjectsEqual(..)`` method that compares individual
fields of the Result object and yields better, easy to understand
messages in case of an attribute mismatch.

Closes coala#4302

gitmate-bot added a commit to yash-nisar/coala that referenced this issue Feb 21, 2018

LocalBearTestHelper: Add ``assertObjectsEqual``
Add ``assertObjectsEqual(..)`` method that compares individual
fields of the Result object and yields better, easy to understand
messages in case of an attribute mismatch.

Closes coala#4302

@gitmate-bot gitmate-bot closed this in #4310 Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment