Enable WireClient tests #1800

narrieta · 2020-03-06T21:22:14Z

Re-enables the tests disabled by #1799

I'll annotate the PR with the relevant changes.

This change is

codecov · 2020-03-06T21:24:12Z

Codecov Report

Merging #1800 into develop will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #1800      +/-   ##
===========================================
+ Coverage    68.49%   68.54%   +0.05%     
===========================================
  Files           82       82              
  Lines        11734    11729       -5     
  Branches      1644     1644              
===========================================
+ Hits          8037     8040       +3     
+ Misses        3349     3343       -6     
+ Partials       348      346       -2

Impacted Files	Coverage Δ
azurelinuxagent/common/protocol/wire.py	`71.4% <ø> (+0.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f917863...f855715. Read the comment docs.

narrieta · 2020-03-06T21:25:22Z

azurelinuxagent/common/protocol/wire.py

@@ -520,11 +520,6 @@ def __init__(self, endpoint):
        logger.info("Wire server endpoint:{0}", endpoint)
        self._endpoint = endpoint
        self._goal_state = None
-        self._hosting_env = None


I forgot to remove these on #1777

tests/protocol/mock_wire_protocol.py

narrieta · 2020-03-06T21:32:56Z

tests/protocol/test_hostplugin.py

@@ -33,7 +33,7 @@
 from azurelinuxagent.common.utils import restutil
 from tests.protocol import mock_wire_protocol
 from tests.protocol.mockwiredata import WireProtocolData, DATA_FILE, DATA_FILE_NO_EXT
-from tests.protocol.test_wire import MockResponse
+from tests.protocol.test_wire import MockResponse as TestWireMockResponse


This one was so much fun to debug! This test uses 2 entities named MockResponse: the one imported here and another at the bottom of the file.

Which one gets used depends on whether patch is used as a decorator or as a statement. Ah, the tangled web we weave!

The import alias takes care of this issue.

narrieta · 2020-03-06T21:42:25Z

tests/protocol/test_hostplugin.py

@@ -403,6 +403,7 @@ def test_validate_http_request(self):

        with mock_wire_protocol.create(DATA_FILE) as protocol:
            test_goal_state = protocol.client._goal_state
+            plugin = protocol.client.get_host_plugin()


Since now get_host_plugin() always fetches the goal state, this initialization was breaking the code below under the scope of the http_request mock.

I may come back (and also ask your help) to update more tests. Similar issues are all over the code.

narrieta · 2020-03-06T21:43:52Z

tests/protocol/test_hostplugin.py

-           return_value=MockResponse(status_code=200, body=b''))
-    @patch("azurelinuxagent.common.protocol.healthservice.HealthService.report_host_plugin_versions")
-    def test_ensure_health_service_called(self, patch_http_get, patch_report_versions):
+        with patch("azurelinuxagent.common.utils.restutil.http_get") as patch_http_get:


I moved these patch decorators to be statements while I was debugging the issue with the mock response. Although this change is no longer needed I'm leaving there should we need to debug more.

narrieta · 2020-03-06T21:45:59Z

tests/protocol/test_wire.py

-        """
-        with create_mock_protocol(status_upload_blob=testurl, status_upload_blob_type=testtype) as protocol:
-            protocol.client.status_blob.vm_status = VMStatus(message="Ready", status="Ready")
+    def test_upload_status_blob_should_use_the_host_channel_by_default(self, *_):


The changes below are the meat of this PR. Now we mock the interaction with the host/storage in terms of http requests, instead of reaching into implementation internals.

pgombar

Thanks a lot for this, the tests look so much simpler and it's easier to keep track of what's happening in them.

pgombar · 2020-03-06T23:11:47Z

tests/protocol/test_wire.py

-                        ret = client.fetch_manifest([VMAgentManifestUri(uri="uri1")])
-                        self.assertEquals(ret, "OK")
+        with mock_wire_protocol.create(mockwiredata.DATA_FILE) as protocol:
+            protocol.client.get_host_plugin()  # force initialization of the host plugin


Why is this needed here and not in the test above?

It is not needed, but simplifies the mock below. Initialization of the host plugin does a request for the goal state. Since the mock below is tracking goal state requests I would need to add code to filter out this one request. It is not too hard, but adding that code there would make the code less readable since it would distract from the actual intention of the test.

Some of the other tests don't track goal state requests so I do it only when it simplifies the code.

Thanks, I'll add a comment.

pgombar · 2020-03-06T23:29:33Z

tests/protocol/test_wire.py

-        mock_failed_response = MockResponse(body=b"", status_code=httpclient.GONE)
-        mock_successful_response = MockResponse(body=b"OK", status_code=200)
+                self.assertEquals(manifest, manifest_xml)
+                self.assertTrue(any(args[0][0] == 'GET' and args[0][1] == manifest_url for args in mock_request.call_args_list))  # direct channel


Don't we expect the first GET to succeed since we're mocking it? Why are we expecting several requests?

Note that this test follows a different pattern than the other ones. This one was one of the first tests I started rewriting.

The underlying implementation of the protocol can trigger multiple requests while executing those fetch* and upload* methods. For example, if we get to use the host plugin we will trigger a request for the goal state, followed by puts to the health service for the plugin, etc.

The mocks in the other tests keep track of the interesting requests in http_request.urls and discard the rest. In this one test I am using mock_request.call_args_list instead. If you run the test under the debugger and set a breakpoint on the first assert you can check the call list and you will see a request for the goal state on top of the the GET for the manifest.

I think the pattern I used on the other tests allows for stronger assertions, and also makes tests a lot more readable so I'll change this one to follow that pattern as well.

Also, post-release, I'll see if I can capture that pattern in a more general tool that we can use for this kind of tests.

I captured the pattern I was mentioning as mock_http_request in mocks.py

Got it, thanks for the update.

narrieta · 2020-03-09T15:02:50Z

tests/common/test_event.py

@@ -28,7 +28,8 @@
    WALAEventOperation, parse_xml_event, parse_json_event, AGENT_EVENT_FILE_EXTENSION, EVENTS_DIRECTORY
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.protocol.goal_state import GoalState
-from tests.protocol import mockwiredata, mock_wire_protocol
+from tests.protocol import mockwiredata
+from tests.protocol.mocks import mock_wire_protocol


I moved mock_wire_protocol to a different file (mocks.py)

I also changed the usage from

with mock_wire_protocol.create(...)...

to

with mock_wire_protocol(...)...

As I'm adding more functionality to the mocks the latter feels better.

Yup - Looks cleaner. Thanks

narrieta · 2020-03-09T15:09:26Z

tests/protocol/mock_wire_protocol.py

-from tests.protocol.mockwiredata import WireProtocolData
-
-@contextlib.contextmanager
-def create(mock_wire_data_file):


moved this to mocks.py as mock_wire_protocol

narrieta · 2020-03-09T15:11:23Z

tests/protocol/mocks.py

+
+
+@contextlib.contextmanager
+def mock_wire_protocol(mock_wire_data_file):


Moved here from the previous mock_wire_protocol.py.

Also, I changed it to mock 1 level deeper (http_request instead of http_get) to address all the scenarios in the tests.

narrieta · 2020-03-09T15:13:25Z

tests/protocol/mocks.py

+        def http_request(method, url, data, headers=None, use_proxy=False, max_retry=restutil.DEFAULT_RETRIES, retry_codes=restutil.RETRY_CODES, retry_delay=restutil.DELAY_IN_SECONDS):
+            if method == 'GET' and mock_data_re.match(url) is not None:
+                return protocol.mock_wire_data.mock_http_get(url, headers, use_proxy, max_retry, retry_codes, retry_delay)
+            elif method == 'POST':


added support for POST

narrieta · 2020-03-09T15:13:56Z

tests/protocol/mocks.py

+            stop_mock_crypt_util.mock = None
+    stop_mock_crypt_util.mock = None
+
+    def stop():


those stop functions are mainly for debugging purposes; they come handy when there are conflicts with the mocks used by tests and the ones used here.

narrieta · 2020-03-09T15:15:14Z

tests/protocol/mocks.py

+
+
+@contextlib.contextmanager
+def mock_http_request(http_get_handler=None, http_post_handler=None, http_put_handler=None):


New mock useful for tests that need to interact with the host/storage at the HTTP request level.

I'll probably merge it with mock_wire_protocol since most test scenarios likely will use mock_http_request nested within mock_wire_protocol. Will see first what kind of usage this mock gets in other tests.

vrdmr

minors; LGTM.

vrdmr · 2020-03-09T20:01:12Z

tests/common/test_event.py

@@ -28,7 +28,8 @@
    WALAEventOperation, parse_xml_event, parse_json_event, AGENT_EVENT_FILE_EXTENSION, EVENTS_DIRECTORY
 from azurelinuxagent.common.future import ustr
 from azurelinuxagent.common.protocol.goal_state import GoalState
-from tests.protocol import mockwiredata, mock_wire_protocol
+from tests.protocol import mockwiredata
+from tests.protocol.mocks import mock_wire_protocol


Yup - Looks cleaner. Thanks

vrdmr · 2020-03-09T20:25:42Z

tests/protocol/test_wire.py

+                self.assertEquals(success, True, 'The download should have succeeded')
+                self.assertEquals(len(urls), 2, "Unexpected number of HTTP requests: [{0}]".format(urls))
+                self.assertEquals(urls[0], extension_url, "The first attempt should have been over the direct channel")
+                self.assertTrue(urls[1].endswith('/extensionArtifact'), "The retry attempt should have been over the host channel")


Do we need to validate the IP address to validate it went through the HGAP? I know the /extensionArtifact should be good enough to validate if the correct URI was hit.

yes, that is one of the changes i want to do post-release

pgombar

LGTM

Enable WireClient tests

d186347

narrieta requested review from larohra, pgombar and vrdmr as code owners March 6, 2020 21:22

narrieta commented Mar 6, 2020

View reviewed changes

tests/protocol/mock_wire_protocol.py Outdated Show resolved Hide resolved

narrieta commented Mar 6, 2020

View reviewed changes

tests/protocol/mock_wire_protocol.py Outdated Show resolved Hide resolved

narrieta commented Mar 6, 2020

View reviewed changes

tests/protocol/mock_wire_protocol.py Outdated Show resolved Hide resolved

narrieta commented Mar 6, 2020

View reviewed changes

pgombar reviewed Mar 6, 2020

View reviewed changes

narrieta added 3 commits March 7, 2020 15:57

Review feedback

9b83701

Created mock_http_request

fbd87d9

Remove import

cb1a1e2

narrieta commented Mar 9, 2020

View reviewed changes

Use kwargs

f855715

narrieta requested a review from pgombar March 9, 2020 16:42

vrdmr approved these changes Mar 9, 2020

View reviewed changes

pgombar approved these changes Mar 9, 2020

View reviewed changes

narrieta merged commit 2157714 into Azure:develop Mar 9, 2020

narrieta deleted the enable-tests branch March 9, 2020 20:44

narrieta mentioned this pull request Mar 12, 2020

Handling errors while reading extension status files (Limiting Size and Transient issues) #1761

Merged

6 tasks

pgombar mentioned this pull request Mar 20, 2020

Update test_wire unit tests to use mock_wire_protocol #1827

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable WireClient tests #1800

Enable WireClient tests #1800

narrieta commented Mar 6, 2020 •

edited by vrdmr

Loading

codecov bot commented Mar 6, 2020 •

edited

Loading

narrieta Mar 6, 2020

narrieta Mar 6, 2020 •

edited

Loading

narrieta Mar 6, 2020

narrieta Mar 6, 2020 •

edited

Loading

narrieta Mar 6, 2020

pgombar left a comment

pgombar Mar 6, 2020

narrieta Mar 7, 2020 •

edited

Loading

pgombar Mar 6, 2020

narrieta Mar 7, 2020 •

edited

Loading

narrieta Mar 9, 2020

pgombar Mar 9, 2020

narrieta Mar 9, 2020

narrieta Mar 9, 2020

vrdmr Mar 9, 2020

narrieta Mar 9, 2020

narrieta Mar 9, 2020 •

edited

Loading

narrieta Mar 9, 2020

narrieta Mar 9, 2020

narrieta Mar 9, 2020 •

edited

Loading

vrdmr left a comment

vrdmr Mar 9, 2020

vrdmr Mar 9, 2020

narrieta Mar 9, 2020

pgombar left a comment



		@contextlib.contextmanager
		def mock_wire_protocol(mock_wire_data_file):



		@contextlib.contextmanager
		def mock_http_request(http_get_handler=None, http_post_handler=None, http_put_handler=None):

Enable WireClient tests #1800

Enable WireClient tests #1800

Conversation

narrieta commented Mar 6, 2020 • edited by vrdmr Loading

codecov bot commented Mar 6, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

narrieta Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

vrdmr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

narrieta commented Mar 6, 2020 •

edited by vrdmr

Loading

codecov bot commented Mar 6, 2020 •

edited

Loading

narrieta Mar 6, 2020 •

edited

Loading

narrieta Mar 6, 2020 •

edited

Loading

narrieta Mar 7, 2020 •

edited

Loading

narrieta Mar 7, 2020 •

edited

Loading

narrieta Mar 9, 2020 •

edited

Loading

narrieta Mar 9, 2020 •

edited

Loading