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

Make compile_failure report the case in a clearer way #61

Open
zormit opened this issue Aug 5, 2024 · 1 comment · May be fixed by #62
Open

Make compile_failure report the case in a clearer way #61

zormit opened this issue Aug 5, 2024 · 1 comment · May be fixed by #62
Labels
enhancement New feature or request

Comments

@zormit
Copy link

zormit commented Aug 5, 2024

This would be nice-to-have, so I might take it on myself, but I'd be curious what you think about some nicer test output. I still have to form an opinion on how it should look like, but I think the existing one, in the case of compile_failure is relatively noisy and not super helpful. At least it does the most important thing and refer to the file:

Traceback (most recent call last):
  File "/Users/mn/code/writing-ccompiler/writing-a-c-compiler-tests/test_framework/basic.py", line 586, in test_invalid
    self.compile_failure(program)
  File "/Users/mn/code/writing-ccompiler/writing-a-c-compiler-tests/test_framework/basic.py", line 340, in compile_failure
    with self.assertRaises(
AssertionError: Didn't catch error in /Users/mn/code/writing-ccompiler/writing-a-c-compiler-tests/tests/chapter_1/invalid_parse/unclosed_paren.c

I think it could improved as follows:

  • the assertion error traceback isn't important to the user of the testsuite as it doesn't point to an interesting place. Maybe catching it and repackaging it could be a way to make it less noisy?
  • also I personally would find a relative path that contains the more relevant bits more readable. In this case chapter_1/invalid_parse/unclosed_paren.c. Here I'm assuming the user doesn't have multiple copies of testsuites. Upside of the absolute path is that you can copy it into any other place without having to worry where it is.
  • I'm not sure if that's too much output, it could be a flag for the testsuite, but I imagine it could also be helpful to print the content of the invalid file?

What do you think about this in general?

@zormit zormit changed the title Make compile_failure report the problem in a nicer way Make compile_failure report the case in a clearer way Aug 5, 2024
@nlsandler nlsandler added the enhancement New feature or request label Aug 5, 2024
@nlsandler
Copy link
Owner

nlsandler commented Aug 5, 2024

Yes, the output could definitely be more readable here!

  • I 100% agree with hiding the traceback. I've dug into this a little bit, and I think the right approach would be to subclass TextTestResult to not display the traceback, rather than trying to explicitly hide the exception. (I might want a flag to re-enable tracebacks, mostly to make debugging the test suite itself easier, but they definitely don't need to be displayed by default.)

  • I agree that a relative path would be nicer here.

  • Printing the contents of the invalid file would be too noisy to do by default - especially in later chapters where the tests get longer. I'm not opposed to having a flag to enable it, though. We could give it a try in a branch and see how we like it.

If you want to tackle some or all of these improvements, let me know - otherwise I'll take them on as I have time.

@zormit zormit linked a pull request Aug 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants