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 & IS-14 Test Suites #810

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

IS-12 & IS-14 Test Suites #810

wants to merge 110 commits into from

Conversation

jonathan-r-thorpe
Copy link
Contributor

@jonathan-r-thorpe jonathan-r-thorpe commented May 9, 2023

  • Initial commit
  • Added IS-12 spec to Config
  • Updated specs for IS-12-01 test suite

* Initial commit
* Added IS-12 spec to Config
* Updated specs for IS-12-01 test suite
Co-authored-by: jonathan-r-thorpe <[email protected]>
Co-authored-by: lo-simon <[email protected]>
Co-authored-by: Gareth Sylvester-Bradley <[email protected]>
@jonathan-r-thorpe
Copy link
Contributor Author

Carrying over the unresolved discussion around whether URL path of IS-12 specs should follow what other specs have done #800 (comment)

and comment about where "selector" and newly introduced "websocket" parameters are best located #800 (comment)

jonathan-r-thorpe and others added 5 commits June 21, 2023 09:39
* Initial commit
* Added IS-12 spec to Config
* Updated specs for IS-12-01 test suite
* Implement NCP endpoint and create WebSocket tests
* Updated API name
* Updated test definition.
* Improve control endpoint checking
* Use of compare_urls to test Control Protocol `href`
* Corrected formatting. Improved WebSocket handling
* Refactored control API checking
* Corrected failure case in test_02
* Use NMOSUtils.do_test_device_control
* Test WebSocket services and controls are as secure as HTTP endpoints
* Fix protocol bug, remove stripping of trailing slash
* Add is_message_received to WebsocketWorker
* Added IS12Utils
* Add Root Block tests
* Check Root Block and check Class Manager OID and Role
---------
Co-authored-by: lo-simon <[email protected]>
Co-authored-by: Gareth Sylvester-Bradley <[email protected]>
…pe definitions (#819)

Auto validation of classes and datatypes
Testing of error conditions
Validation of device model against advertised classes and datatypes
Checking of Managers
Checking that roles are unique within a block
Checking that oids are unique

Co-authored-by: lo-simon <[email protected]>
Co-authored-by: Gareth Sylvester-Bradley <[email protected]>
[IS-04-01] Much smaller zeroconf monkey patch

(cherry picked from commit aa58b5e)
* Fixed error reporting
* Added Set user label test
* Validate touchpoints
* Validate NcTouchpointNmos or NcTouchpointNmosChannelMapping depending on contextNamespace
* Sequence command JSON helper functions
* Build device model object tree when validating device model
* Check FindMembersByRole
* Recursively check each block's role paths
* find members by role test
* Find members by class id test
* GetMemberDescriptors test
* removed protocolVersion
* GetSequenceLength test
* Subscribe to all objects and test changing all user labels
* Add Class Manager method tests
* Added illegal command handle test
* IndexOutOfBounds test. WebSocket kept open test
* Updated comments and links to specification
---------
Co-authored-by: Gareth Sylvester-Bradley <[email protected]>
Co-authored-by: Gareth Sylvester-Bradley <[email protected]>
* Specify explicit URL path for WebSocket endpoint

* Default to array, not dict

* Linting tweaks

* Handle leading slash on URL path parameter

* Update comment. Rename helper function

* Rollback tentative changes
@jonathan-r-thorpe jonathan-r-thorpe marked this pull request as ready for review August 8, 2023 10:43
Copy link
Contributor

@cristian-recoseanu cristian-recoseanu left a comment

Choose a reason for hiding this comment

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

I just wanted to say that from a "product owner" point of view I have been using the test suite regularly and I am confident that it satisfies the testing plan the activity group set out. I want to congratulate and thank @jonathan-r-thorpe for all the hard work that went into this.

The two node implementations we have access to are passing the relevant tests succesfully.

NMOS Tests 2

if parsed_message["messageType"] == MessageTypes.SubscriptionResponse:
results.append(parsed_message["subscriptions"])
if parsed_message["messageType"] == MessageTypes.Notification:
self.notifications += parsed_message["notifications"]
Copy link

Choose a reason for hiding this comment

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

The current IS-12 testing seems to assume that notifications received from a websocket are synchronous with the sending of commands which to my opinion is erroneous. Both paths to my understanding are totally asynchronous, requiring the use of the handle to synchronize responses with commands, possibly receiving notifications before and/or after command responses.

The notification cannot be retrieved only from def send_command(self, test, command_json) and this function must not assume any ordering between command responses and notifications.

For example if messages = self.ncp_websocket.get_messages() gets a notification before the command response, the send_command function will fail before it thinks there is no result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are correct that is making assumptions about the notifications being synchronous and that the handle should be used to associate a command with its command response.

I will modify to filter the messages on the command handle which should mitigate these problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example if messages = self.ncp_websocket.get_messages() gets a notification before the command response, the send_command function will fail before it thinks there is no result.

Yes, there is a risk of this. This code should really loop until the corresponding response has been received, or a timeout is reached.

Copy link

Choose a reason for hiding this comment

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

Here is what I used in the interim ... The expecting_notifications flag is set when we start using notifications (not a perfect solution). It works only if no more than one notification is expected after sending a command. It works with the current test suite but could fail with more elaborate use of notifications.

` def send_command(self, test, command_json):
"""Send command to Node under test. Returns [command response]. Raises NMOSTestException on error"""
# IS-12 Check message type, check handle numeric identifier
# https://specs.amwa.tv/is-12/branches/v1.0/docs/Protocol_messaging.html#command-message-type
# MS-05-02 All methods MUST return a datatype which inherits from NcMethodResult.
# When a method call encounters an error the return MUST be NcMethodResultError
# or a derived datatype.
# https://specs.amwa.tv/ms-05-02/branches/v1.0/docs/Framework.html#ncmethodresult

    # Assume single command
    command_handle = command_json['commands'][0]['handle'] if command_json.get('commands') else 0

    self.ncp_websocket.send(json.dumps(command_json))

    results = []
    start_time = time.time()
    while time.time() < start_time + WS_MESSAGE_TIMEOUT:
        if self.ncp_websocket.is_messages_received():
            messages = self.ncp_websocket.get_messages()

            # find the response to our request
            for message in messages:
                parsed_message = json.loads(message)

                if self.message_type_to_schema_name(parsed_message["messageType"]):
                    self.validate_is12_schema(
                        test,
                        parsed_message,
                        self.message_type_to_schema_name(parsed_message["messageType"]),
                        context=self.message_type_to_schema_name(parsed_message["messageType"]) + ": ")
                else:
                    raise NMOSTestException(test.FAIL("Unrecognised message type: " + parsed_message["messageType"],
                                                    "https://specs.amwa.tv/is-12/branches/{}"
                                                    "/docs/Protocol_messaging.html#command-message-type"
                                                    .format(self.spec_branch)))

                if parsed_message["messageType"] == MessageTypes.CommandResponse:
                    responses = parsed_message["responses"]
                    for response in responses:
                        if response["handle"] == command_handle:
                            if response["result"]["status"] != NcMethodStatus.OK:
                                raise NMOSTestException(test.FAIL(response["result"]))
                            results.append(response)
                if parsed_message["messageType"] == MessageTypes.SubscriptionResponse:
                    results.append(parsed_message["subscriptions"])
                if parsed_message["messageType"] == MessageTypes.Notification:
                    self.notifications += parsed_message["notifications"]
                if parsed_message["messageType"] == MessageTypes.Error:
                    raise NMOSTestException(test.FAIL(parsed_message, "https://specs.amwa.tv/is-12/branches/{}"
                                                    "/docs/Protocol_messaging.html#error-messages"
                                                    .format(self.spec_branch)))
                
            if not self.expecting_notifications and len(results) != 0:
                break
            if self.expecting_notifications and len(results) != 0 and len(self.notifications) != 0:
                break

        time.sleep(0.2)
                
    if len(results) == 0:
        raise NMOSTestException(test.FAIL("No Message Response received.",
                                          "https://specs.amwa.tv/is-12/branches/{}"
                                          "/docs/Protocol_messaging.html#command-message-type"
                                          .format(self.spec_branch)))

    if len(results) > 1:
        raise NMOSTestException(test.FAIL("Received multiple responses : " + len(responses)))

    return results[0]`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think this is approaching a solution.

Although, I'm wondering whether we should somehow decouple the Notification handling from the Command handling. At the moment the assumption is that when the tests update the Device Model of the Node Under Test, any subscription notification will arrive at the same time as the CommandResponse and can therefore be polled using self.ncp_websocket.get_messages().

However, this may well be an unsafe assumption if the notification happens even a short time after the CommandResponse is returned.

Currently the incoming messages are cached in the WebsocketWorker until they are polled. However, it might be better to attach a handler to the websocket and validate/sort the messages as they arrive. Perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although I guess that is the problem that the expecting_notifications flag is solving.

I will update the code in a separate PR according to your suggestion for your review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alabou I have now updated the code according to your suggestion.

Copy link

Choose a reason for hiding this comment

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

Looks good to me for the actual test suite usage of notifications. If some day we need to test for notifications that are totally asynchronous with respect to commands then we'll need to revisit this. Thanks.

jonathan-r-thorpe and others added 12 commits October 6, 2023 09:19
(cherry picked from commit 4caeca4)
Libraries like python-zeroconf are starting to depend on more recent Python features

(cherry picked from commit b10ff71)
So that same one is used by Python tests and testssl.sh tests

Co-authored-by: Simon Lo <[email protected]>
(cherry picked from commit 3f10a8d)
(cherry picked from commit 796a115)
Co-authored-by: Simon Lo <[email protected]>
(cherry picked from commit 43a1f76)
Co-authored-by: jonathan-r-thorpe <[email protected]>
(cherry picked from commit 215f736)
Move MS-05 utility functions into MS05Utils
Added MS0502Test and IS1402Test
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.

None yet

5 participants