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-12] Initial Commit for IS-12 Test Suite #800

Merged
merged 20 commits into from
May 9, 2023

Conversation

jonathan-r-thorpe
Copy link
Contributor

Checks that the NCP endpoint can be discovered from Node API of Node under test
Checks that a WebSocket connection can be established

}, {
"spec_key": "is-12",
"api_key": "control",
"disable_fields": ["host", "port"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we'd make/allow the test tool user enter the host/port for the control endpoint and then validate in the "interoperability with IS-04" test case that that is indeed the endpoint advertised for at least one Device in the Node API. Would that work for IS-12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can certainly use the host and port to validate the control endpoint. However, I don't think we can "calculate" the control endpoint href simply from the host and port as the actual href can be arbitrary (so far according to the IS-12 spec e.g. ws://127.0.0.1:8080/x-nmos/ncp/v1.0/connect). AFAICS neither IS-05 or IS-07, for example, constrain the form of the href, or am I missing something there?

Copy link
Contributor

Choose a reason for hiding this comment

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

IS-05 and IS-07 actually both do constrain the URL path.

However, IS-08 doesn't (fully), it introduces the concept of a "selector" which is appended to the defined base path.
You can see how that's represented in the TEST_DEFINITIONS for the IS-08-02 test suite with "selector": True (disabled for the IS-04 API).

However, that's not quite enough for you here, where there's no defined base path, so this codeis wrong:

if api_key in CONFIG.SPECIFICATIONS[spec_key]["apis"]:
url += "x-nmos/{}/".format(api_key)
if endpoints[index]["version"] is not None:
url += "{}/".format(endpoints[index]["version"])
if endpoints[index]["selector"] not in [None, ""]:
url += "{}/".format(endpoints[index]["selector"])

The IS-10-01 suite actually needs the same as IS-12, but that test suite is languishing since the IS-10 spec change to move from a completely fixed path to no defined base path in common with the underlying OAuth 2.0 Authorization Server Metadata spec...

Changing selector from Boolean to an enum None/Append/Replace could work for both IS-12 and IS-10... but really selector ought to be a property of each API so we could handle test suites needing two APIs with different selector modes. But then we need to gather the UI requirements differently... :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that the referenced code in NMOSTesting.py is constructing an http/https url to an API, whereas here we want to construct a ws/wss url - I could add a "websocket": True to the test definition (similar to the "selector": True) so the url can be constructed correctly.

A question for the IS-12 spec is whether the url should (must) be constrained or whether that is too restrictive - I would argue for consistency with IS-08 at least and so the existing selector mechanism would work:

api_key: "ncp",
version: "v1.0",
selector: True,
websocket: True

with parameters captured from the UI:

host: "127.0.0.1"
port: "8080"
API selector: "connect"

Would yield the desired url as specified in the example mock device (ws://127.0.0.1:8080/x-nmos/ncp/v1.0/connect)

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right about protocol for sure. Thinking about addingwebsocket at the same spec level as the current attributes rather than per api clearly demonstrates that it's the wrong level.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking on this, there is a difference between the HTTP endpoints and the WebSocket ones that justifies the former having fixed URL paths and the latter not needing to. The WebSocket endpoint supports a single request, opening a connection over which subsequent 'requests' are made. In the other hand the HTTP URL is a base from which paths to many endpoints are determined. The base path could have been allowed to be anything since the other endpoints could be (and may well be in current implementations) calculated via relative paths, so it's not much of a distinction to be sure!

Copy link
Contributor Author

@jonathan-r-thorpe jonathan-r-thorpe May 10, 2023

Choose a reason for hiding this comment

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

Yes, I see the distinction, and it's a fair point.

I think there are two decisions to be made here:

  1. Should the WebSocket URL have a known form that is consistent with the existing IS HTTP URLs (i.e. ws(s)://<ip address or hostname>:<port>/x-nmos/<api type>/<api version>/[<api selector>/]? My feeling on this is yes for consistency and to make development and deployment easier. Whether an api selector is needed or desirable is an open question.
  2. Should NMOSTesting "calculate" this URL in the same way it calculates the base URL of the HTTP endpoints. Here either we use the existing mechanism with the tweaks suggested in the current PR, or we implement a separate mechanism that is very similar due to the distinction you've highlighted above. Again my feeling is to reuse the existing mechanism as per the current state of the PR.

@jonathan-r-thorpe jonathan-r-thorpe marked this pull request as ready for review May 5, 2023 15:21
@garethsb
Copy link
Contributor

garethsb commented May 6, 2023

See jonathan-r-thorpe#4

nmostesting/NMOSTesting.py Outdated Show resolved Hide resolved
}, {
"spec_key": "is-12",
"api_key": "ncp",
"websocket": True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this is OK for first iteration, but what do you think about whether websocket and selector flags belong in the spec definitions rather than the test suite definitions?

@jonathan-r-thorpe jonathan-r-thorpe force-pushed the is-12-jrt branch 4 times, most recently from 37f3ebd to 16a9b83 Compare May 9, 2023 09:41
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.

3 participants