class library: Check message is >= template size in OSCArgsMatcher #4027
Conversation
…tcher - fixes #1482 Signed-off-by: Scott Wilson <i@scottwilson.ca>
|
Yes, please add some tests for this; tests aren't only for new functionality, i'm not sure how you got that idea. We also add regression tests for fixed bugs. |
|
@brianlheim From somebody else's PR. I'm not sure it's obvious that every bug fix should require a new test, but I'm not opposed. Probably would make sense to just add some OSCFunc tests generally including a test for this. |
|
@brianlheim Is there best practice doc for the current state of UnitTests? |
|
This is the WIP guide for UnitTesting: https://github.com/supercollider/supercollider/wiki/Unit-Testing-Guide |
Yep! I announced it on the list awhile ago; check Developing.md or the PR template you just filled out. :)
How could we make it more obvious?
Yes, that would be great if you don't mind, but a simple regression test to cover this case is all that's needed here. |
I meant things like should we be using UnitTestScript (which I hadn't noticed before just now), best practice for the tests themselves, etc. I did look in those places, but didn't see that info. But @patrickdupuis's link looks like the sort of thing I'm asking about. Sorry, not trying to be a pain, but it's been a while since I wrote one and I would like to do it within expected standards the first time!
Well, maybe instead of saying 'Make sure you have added the necessary tests for your changes', you could add 'All changes to code should include a test which checks for correct functionality, including regression tests for bug fixes.' Just be explicit, so it's clear. |
Sorry, my mistake, I thought we had a link in there. I must have purposefully left it out since my guide is still WIP.
That sounds good to me! Feel free to open a PR that changes .github/PULL_REQUEST_TEMPLATE.md and other relevant documentation (contributing.md, etc.) |
done |
Signed-off-by: Scott Wilson <i@scottwilson.ca>
|
Test added. |
| @@ -0,0 +1,32 @@ | |||
| TestOSCFunc : UnitTest { | |||
|
|
|||
| test_argSizeMatchingSucceeds { | |||
There was a problem hiding this comment.
I'm nitpicking, but I think the tests would be more clearly named as "test_argSizeMatching_succeeds" and "test_argSizeMatching_fails".
There was a problem hiding this comment.
Really? I find that a slightly odd mix of camel case and underscore. Okay there was already test_ at the beginning but I understood that as a convention to support the slightly bodgy way UnitTest identifies test methods.
But I don't mind if you change them, and thanks for reviewing
There was a problem hiding this comment.
This is covered in the guide I wrote, is already used throughout the test suite, and I found it to be something that others recommended in their guides. Descriptive test names often have 2 or 3 components, each of which may be multiple words, so there's no other way to make that clear
There was a problem hiding this comment.
If you disagree with that please make a case for it on the ML or on the gist, it's still a WIP!
There was a problem hiding this comment.
Ah sorry!
Personally I don't find that much more readable than just camel case, but I'm okay with it. I don't generally like when original conventions are diluted with new ones, but this is just the test suite after all, so it could be a special case. I'd prefer not to add lots of thing_camelCase_etc in Common though.
There was a problem hiding this comment.
Yes definitely, this is a convention that should only be kept in test code, in Common almost all (hopefully all) method names are free from underscores
Purpose and Motivation
Fix bug noted in issue #1482 in which OSCArgsMatcher would show a match with fewer args than the template.
Types of changes
Checklist
(There's no new functionality, so I've not added a test for this, though I can if needed)