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

UnitTest: add debug method #3623

Merged
merged 1 commit into from
Dec 28, 2019
Merged

UnitTest: add debug method #3623

merged 1 commit into from
Dec 28, 2019

Conversation

aspiers
Copy link
Contributor

@aspiers aspiers commented Apr 2, 2018

This allows TestCases to provide debug which will be displayed in the results report along with any details from assertions, except for passing tests where brief reporting has been requested via

UnitTest.passVerbosity = UnitTest.brief;

This is very similar to the addDetail method in Python's testtools.

Unfortunately UnitTest doesn't provide isolation between test cases as per #3572, so we have to reset the debug state after each test method, otherwise debug from one test method can bleed into the next if brief reporting is enabled and all the tests in the first one pass.

Partially addresses #3365.

@telephon
Copy link
Member

telephon commented Apr 2, 2018

Do I understand it correctly: the isolation problem you have run into comes from the fact that you need some instance variables that are local to the test-method. This could be achieved by local variables in the method. But you want to access these variables in the context from which the test-method is called.

So you are looking for a good place to store the debug string that grows over several assert statements within one single method?

@mossheim
Copy link
Contributor

mossheim commented Apr 2, 2018

^ That should be a comment on the issue, not on this PR. But no, you could have isolation and still run setup and teardown.

@aspiers
Copy link
Contributor Author

aspiers commented Apr 2, 2018

Replied in #3572.

Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

IIUC resetDebug should be a private method; there's currently no reason for it to be called from within a test, and you've provided no documentation for it while you have for debug. Plus it just seems like the kind of implementation detail we wouldn't want to be part of the public interface. I'm skeptical of calling it from within UnitTestResult.

SCClassLibrary/Common/UnitTesting/UnitTest.sc Outdated Show resolved Hide resolved
HelpSource/Classes/UnitTest.schelp Outdated Show resolved Hide resolved
@telephon
Copy link
Member

telephon commented Apr 2, 2018

I've added an implementation that guarantees isolated methods (#3624). Let's first finish that one and then the debug can be added easily.

@aspiers
Copy link
Contributor Author

aspiers commented Aug 22, 2018

#3836 has fixed the test isolation problem, so this needs to be reworked now.

This allows TestCases to provide debug which will be displayed in the
results report along with any details from assertions, except for
passing tests where brief reporting has been requested via

    UnitTest.passVerbosity = UnitTest.brief;

This is very similar to the addDetail method in Python's testtools:

    https://testtools.readthedocs.io/en/latest/for-test-authors.html#details

Partially addresses supercollider#3365.
Copy link
Contributor

@muellmusik muellmusik left a comment

Choose a reason for hiding this comment

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

This looks fine to me AFAICS. @brianlheim and @telephon are you happy with this?

@mossheim
Copy link
Contributor

@brianlheim and @telephon are you happy with this?

yeah, it looks good to me

@telephon telephon merged commit 2c11376 into supercollider:develop Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: class library SC class library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants