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

IS-11: Test suites for Stream Compatibility Management #728

Open
wants to merge 86 commits into
base: master
Choose a base branch
from

Conversation

garethsb
Copy link
Contributor

@garethsb garethsb commented Nov 4, 2022

... and BCP-005-01 EDID to Receiver Capabilities Mapping

Replaces #716 with an is-11 activity branch in the AMWA-TV/nmos-testing for simpler collaboration. The group can make PRs to extend the test suites onto this branch, which the activity group can review before merging. Others can use this 'stable' activity branch for testing.

…Interaction with IS-04", and "BCP-005-01 EDID to Receiver Capabilities Mapping", test suites
Copy link
Collaborator

@N-Nagorny N-Nagorny left a comment

Choose a reason for hiding this comment

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

I tried it out and found minor problems. One of them is caused by https://github.com/AMWA-TV/nmos-testing/blob/master/nmostesting/GenericTest.py#L405: nmos-testing expects 200 from /inputs/{inputId}/edid/base but the default state is 204.

nmostesting/suites/IS1101Test.py Outdated Show resolved Hide resolved
nmostesting/suites/IS1102Test.py Outdated Show resolved Hide resolved
garethsb and others added 4 commits January 27, 2023 16:24
Merge lastest code from master on is-11
…eivers (4) (#761)

Based on IS-11 Test Plan v0.6
Senders contains tests 2.0, 2.1 and 2.2
Receivers contains tests 4.1 and 4.2

Signed-off-by: ggeorgea <ggeorgea@matrox.com>
Co-authored-by: Gareth Sylvester-Bradley <garethsb@nvidia.com>
# Conflicts:
#	nmostesting/Config.py
@garethsb garethsb marked this pull request as ready for review April 27, 2023 15:43
)
state = response.json()["state"]
if state in ["awating_essence", "no_essence"]:
time.sleep(3)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is maximum 5 x 3 seconds sleep part of the spec? If not, should probably be configurable?

Copy link

Choose a reason for hiding this comment

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

This not not part of the IS-11 specification ... just an expectation that the device somehow is responsive within a few seconds.

gwgeorgea and others added 3 commits June 14, 2023 09:10
Add usage of STABLE_STATE_DELAY from Config.py
A few failure comment clarifications
# Conflicts:
#	nmostesting/suites/IS1101Test.py
@lo-simon
Copy link
Contributor

lo-simon commented Mar 18, 2024

  1. The following tests,
    test_02_02_03_01, test_02_02_03_02, test_02_02_04_01, test_02_02_04_02, test_02_02_05_01, test_02_02_05_02, test_02_02_06_01, test_02_02_06_02, test_02_02_07_01, test_02_02_07_02, test_02_03_05_01, test_02_03_05_02
    cannot be tested alone, data are required to be setup beforehand via the test_02_02_00
    and
    test_02_03_01, test_02_03_02, test_02_03_03, test_02_03_04
    cannot be tested alone, data are required to be setup beforehand via the test_02_03_00

  2. test_02_03_05_01 cannot test the video sender, which has only had one grain_rate constraint in its capability set

  3. test_02_03_05_02 cannot test the audio sender, which has only had one sample_rate constraint in its capability set

Comment on lines +2313 to +2314
if response.status_code == 422:
print("Device does not accept grain_rate constraint")
Copy link
Contributor

Choose a reason for hiding this comment

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

This code will never be executed

Comment on lines 2542 to 2544
"""
Verify for inputs supporting EDID that the version and the effective EDID change when applying constraints
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

This has the same test description as test_02_03_05_01. What's different?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name one with video and the other one with audio

Copy link

Choose a reason for hiding this comment

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

2.3.5.1 is video, 2.3.5.2 is audio

Copy link
Contributor

@jonathan-r-thorpe jonathan-r-thorpe Mar 20, 2024

Choose a reason for hiding this comment

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

Suggested change
"""
Verify for inputs supporting EDID that the version and the effective EDID change when applying constraints
"""
"""
Verify for audio inputs supporting EDID that the version and the effective EDID change when applying constraints
"""

Bear in mind that these test descriptions are presented in the NMOS Testing user interface, so by having identical descriptions it makes it difficult for a user to disambiguate one test from the other.

return test.PASS()
return test.UNCLEAR("No resources found to perform this test.")

def test_02_03_05_02(self, test):
Copy link
Contributor

Choose a reason for hiding this comment

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

The test numbering seems very convoluted. AFAIK test names should be of the form test_XX. Where they are named test_XX_XX this is where additional tests have been added at a later date...

Copy link
Collaborator

Choose a reason for hiding this comment

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

A compromise that takes into account the existing sectioning: test_XX: Senders ... naming (turning subsection numbers into word prefixes).

Comment on lines 2587 to 2608
valid, response = TestHelper.do_request(
"GET", self.compat_url + "inputs/" + input_id + "/properties/"
)
if not valid:
return test.FAIL(
"Unexpected response from the streamcompatibility API: {}".format(
response
)
)
if response.status_code != 200:
return test.FAIL(
"The input {} properties streamcompatibility request has failed: {}".format(
input_id, response
)
)
try:
version = response.json()["version"]
except json.JSONDecodeError:
return test.FAIL("Non-JSON response returned from Node API")
except KeyError as e:
return test.FAIL("Unable to find expected key: {}".format(e))
self.version[input_id] = version
Copy link
Contributor

@jonathan-r-thorpe jonathan-r-thorpe Mar 19, 2024

Choose a reason for hiding this comment

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

This seems to be a repeated pattern through this test. Perhaps factoring out a helper function would allow this test to be more compact?


default_edid = self.get_effective_edid(test, input_id)

self.another_grain_rate_constraints[sender_id] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

another_grain_rate_constraints only appears to be used locally within this test. Why is it a class variable?

Comment on lines +518 to +531
def get_another_grain_rate(self, grain_rate):
numerator = grain_rate["numerator"]
denominator = grain_rate["denominator"]
if (numerator == 30 or numerator == 25) and denominator == 1:
return {"numerator": numerator * 2, "denominator": 1}
if (numerator == 60 or numerator == 50) and denominator == 1:
return {"numerator": numerator / 2, "denominator": 1}
if numerator == 24 and denominator == 1:
return {"numerator": 30, "denominator": 1}
if (numerator == 30000 or numerator == 25000) and denominator == 1001:
return {"numerator": numerator * 2, "denominator": 1001}
if (numerator == 60000 or numerator == 50000) and denominator == 1001:
return {"numerator": numerator / 2, "denominator": 1001}
return "grain_rate not valid"
Copy link
Contributor

Choose a reason for hiding this comment

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

get_another_grain_rate assumes that the Node under test supports all these grain rates.
Similarly get_another_sample_rate assumes the Node supports all the defined sample rates.

Copy link

Choose a reason for hiding this comment

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

If the node does not support the grain rate is will refuse the constraint and the test should be skipped (once the implementer fix the previously observed bug when testing the return status :). Note that there is yet no Sender Capabilities in NMOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our experience is that the test aren't being skipped ('Could Not Test') but being failed when the Node doesn't support a particular grain rate...

Comment on lines +2662 to +2663
if response.status_code == 422:
print("Device does not accept grain_rate constraint")
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants