-
Notifications
You must be signed in to change notification settings - Fork 48
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
Is-13 add basic API tests #842
base: master
Are you sure you want to change the base?
Conversation
Successfully tested against: garethsb/nmos-cpp@04ac4fd AMWA-TV/is-13@8655e0c
When possible, TestResult.detail should be displayed in stderr since the raised exception prevents from the report creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pat demonstrated this on a resource labelling call and it looks good
For each resource&objects: - Read initial value and store - Reset default value, check timestamp and store - Write max-length and check value+timestamp - Write >max-length and check value+timestamp - Reset default value and compare - Restore initial value Apply to objects: label, description, tags TODO: check IS04
This seems to impact the version(timestamp) test.
@garethsb @jonathan-r-thorpe please, can you have a 1st look at this? |
nmostesting/suites/IS1301Test.py
Outdated
RESOURCES = ["self", "devices", "senders", "receivers"] | ||
OBJECTS = ["label", "description", "tags"] | ||
|
||
# const for label&description-related tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# const for label&description-related tests | |
# Constants for label- and description-related tests | |
nmostesting/suites/IS1301Test.py
Outdated
STRING_OVER_MAX_VALUE = ''.join(['X' for i in range(100)]) | ||
STRING_MAX_VALUE = STRING_OVER_MAX_VALUE[:64] # this is the max length tolerated | ||
|
||
# const for tags-related tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# const for tags-related tests | |
# Constants for tags-related tests | |
nmostesting/suites/IS1301Test.py
Outdated
STRING_OVER_MAX_VALUE = ''.join(['X' for i in range(100)]) | ||
STRING_MAX_VALUE = STRING_OVER_MAX_VALUE[:64] # this is the max length tolerated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter naming here gives the first impression these are number values (cf. INT_MAX).
Perhaps add LENGTH
in as well STRING_MAX_LENGTH_VALUE
? A little verbose but less confusing.
Similarly STRING_OVER_MAX_VALUE
could be STRING_LENGTH_OVER_MAX_VALUE
or STRING_OVER_MAX_LENGTH_VALUE
nmostesting/suites/IS1301Test.py
Outdated
TAGS_OVER_MAX_VALUE = {'location': ['underground'], 'studio': ['42'], 'tech': ['John', 'Mike']} | ||
TAGS_MAX_VALUE = TAGS_OVER_MAX_VALUE.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment for STRING_OVER_MAX_VALUE applies here also.
nmostesting/suites/IS1301Test.py
Outdated
""" | ||
FAKE_ORIG = { | ||
'description': 'fake_orig_desc', | ||
'label': 'fake_orig_label', | ||
'tags': { | ||
'location': ['fake_location'] | ||
} | ||
} | ||
for resource in RESOURCES: | ||
url = "{}{}{}".format(self.annotation_url, 'node/', resource) | ||
TestHelper.do_request("PATCH", url, json=FAKE_ORIG) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant code needs removing.
nmostesting/suites/IS1301Test.py
Outdated
def log(self, msg): | ||
print(msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally the test suites don't print to the console as the console reporting is handled by the testing framework, so the "logging" is best removed.
nmostesting/suites/IS1301Test.py
Outdated
def strip_tags(tags): | ||
if TAGS_TO_BE_SKIPPED in list(tags.keys()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipped or stripped? TAGS_TO_BE_STRIPPED
perhaps?
nmostesting/suites/IS1301Test.py
Outdated
else: | ||
return False, "GET Resquest FAIL" | ||
|
||
def compare_resource(self, object, new, resp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess resp
here is response, which doesn't make much sense in the context of a comparison function.
Would prefer this to be consistent with other compare functions in the code base (e.g. NMOSUtils.compare_urls
, NMOSUtils.compare_api_version
etc.) that, (rightly or wrongly) use (url1, url2)
and (ver1, ver2)
, so (res1, res2)
might be preferable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also object
both an overly broad and overloaded term; is there something more descriptive that could be used here? tag_type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree "object" (and OBJECTS
for the list of them) etc. seems confusing terminology. I think we're talking about what IS-13 calls the "annotation properties"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also agree new
and resp
are confusing, but this function doesn't seem symmetric, which means it's not a usual comparison function... 😕 @jonathan-r-thorpe, let's discuss while we're face-to-face this week!
nmostesting/suites/IS1301Test.py
Outdated
""" Patch a resource with one ore several object values """ | ||
|
||
object = list(new.keys())[0] | ||
valid, resp = TestHelper.do_request("PATCH", url, json=new) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valid, resp = TestHelper.do_request("PATCH", url, json=new) | |
valid, resp = self.do_request("PATCH", url, json=new) | |
nmostesting/suites/IS1301Test.py
Outdated
if not valid: | ||
return False, "PATCH Resquest FAIL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than pass back a flag and message, you could raise NMOSTestException
s in this function. (See do_receiver_put
in IS0401Test.py
for some examples of wrapping the test failure in this exception). You could then remove the flag handling code in do_test
making it more compact and readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'd like to see this done consistently with the rest of the test suites, and functions named in a way which indicates whether they return TestResult
s, raise NMOSTestException
s, or a boolean and (optional) strings.
nmostesting/suites/IS1301Test.py
Outdated
def get_url(self, base_url, resource): | ||
""" | ||
Build the url for both annotation and node APIs which behaves differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting or building? looks more a build to me; idiomatically "create" is used a lot in the other test suites for this sort of thing create_url
perhaps?
nmostesting/suites/IS1301Test.py
Outdated
The script implements the IS-13 test suite as specified by the nmos-resource-labelling workgroup. | ||
At the end of the test, the initial state of the tested unit is supposed to be restored but this | ||
cannot be garanteed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing the spec is the true "source of truth" here. Where possible it would be great to tie test cases back to normative language in the specifications (you can supply a link=
to the test failure case that will embed a url link in the test results seen by the user).
The script implements the IS-13 test suite as specified by the nmos-resource-labelling workgroup. | |
At the end of the test, the initial state of the tested unit is supposed to be restored but this | |
cannot be garanteed. | |
The script implements the IS-13 test suite according to the AMWA IS-13 NMOS Annotation Specification (https://specs.amwa.tv/is-13/). | |
At the end of the test, the initial state of the tested unit is supposed to be restored but this | |
cannot be guaranteed. | |
A general question - looking at the specification there are a few normative references not covered by this test suite. Will these be covered before the activity closes? |
The first spec is not covered indeed, but the second is. I still need to understand the translation of normative |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments, which I'll review with @jonathan-r-thorpe since we're face-to-face today!
nmostesting/suites/IS1301Test.py
Outdated
else: | ||
return False, "GET Resquest FAIL" | ||
|
||
def compare_resource(self, object, new, resp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree "object" (and OBJECTS
for the list of them) etc. seems confusing terminology. I think we're talking about what IS-13 calls the "annotation properties"?
nmostesting/suites/IS1301Test.py
Outdated
except Exception as e: | ||
return False, e.msg | ||
else: | ||
return False, "GET Resquest FAIL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least a typo, but also want to check why this suite needs a function like this if other suites don't, or whether there is already a common approach to this...
return False, "GET Resquest FAIL" | |
return False, "GET Request FAIL" |
nmostesting/suites/IS1301Test.py
Outdated
else: | ||
return False, "GET Resquest FAIL" | ||
|
||
def compare_resource(self, object, new, resp): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also agree new
and resp
are confusing, but this function doesn't seem symmetric, which means it's not a usual comparison function... 😕 @jonathan-r-thorpe, let's discuss while we're face-to-face this week!
nmostesting/suites/IS1301Test.py
Outdated
if not valid: | ||
return False, "PATCH Resquest FAIL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'd like to see this done consistently with the rest of the test suites, and functions named in a way which indicates whether they return TestResult
s, raise NMOSTestException
s, or a boolean and (optional) strings.
nmostesting/suites/IS1301Test.py
Outdated
object = list(new.keys())[0] | ||
valid, resp = TestHelper.do_request("PATCH", url, json=new) | ||
if not valid: | ||
return False, "PATCH Resquest FAIL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return False, "PATCH Resquest FAIL" | |
return False, "PATCH Resuest FAIL" |
nmostesting/suites/IS1301Test.py
Outdated
|
||
def do_test(self, test, resource, object): | ||
""" | ||
Perform the test sequence as documented in the file header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems more natural to see the explanation of this function here to me?
nmostesting/suites/IS1301Test.py
Outdated
# re-GET | ||
valid, resp = self.get_resource(url) | ||
if not valid: | ||
return False, "Get Resquest FAIL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return False, "Get Resquest FAIL" | |
return False, "Get Request FAIL" | |
return self.do_test(test, "receivers", "description") | ||
|
||
def test_04_03(self, test): | ||
"""Annotation test: receivers/sevices/../tags (reset to default, set 5 tags, set >5 tags, check IS04+version)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
return self.do_test(test, "senders", "description") | ||
|
||
def test_03_03(self, test): | ||
"""Annotation test: sender/sevices/../tags (reset to default, set 5 tags, set >5 tags, check IS04+version)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo
return test.PASS() | ||
|
||
def test_01_01(self, test): | ||
""" Annotation test: self/label (reset to default, set 64-byte value, set >64-byte, check IS04+version)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These strings appear in the UI to describe the test, so let's wordsmith somewhat, @jonathan-r-thorpe?
@jonathan-r-thorpe @garethsb all your change requests were addressed. |
Raise exceptions instead of returning tuples. Add reference to the specification when possible.
Default value (1s) increases the duration of the test suite to approx 1min.
Successfully tested against:
garethsb/nmos-cpp@04ac4fd
AMWA-TV/is-13@8655e0c