Pylint integration updates (#643)

* updated files with docstrings to pass pylint Signed-off-by: Mark Cohen <[email protected]> * updated samples to prepare for enabling missing-docstring linter; will continue to work on this before committing setup.cfg Signed-off-by: Mark Cohen <[email protected]> * removed missing-function-docstring from setup.cfg so the linter doesn't fail while work on docstrings continues Signed-off-by: Mark Cohen <[email protected]> * corrected unnecessary return docstring values Signed-off-by: Mark Cohen <[email protected]> * fixing failure in 'black' on reformatting Signed-off-by: Mark Cohen <[email protected]> * updated utils to pass missing-function-docstring tests Signed-off-by: Mark Cohen <[email protected]> * updated functions with missing docstrings or pylint ignore instructions; added a utility to automatically add these ignore instructions to most functions that should be self-describing; rolled back some automatically generated code mistakenly changed Signed-off-by: Mark Cohen <[email protected]> * * ignoring opensearchpy for pylint and then added it back to noxfile.py * fixed some lints; created a feature flag for newer dynamic pylint so now lints can be fixed first in legacy code and then enabled by multiple people * extracted a method for per-folder linting * updated noxfile.lint_per_folder with type hints * enabled unspecified-encoding in pylint * added disable missing-function-docstring pragma to test_clients.py in test_async and test_server * added more encodings to pass unspecified-encoding pylint tests * updated changelog Signed-off-by: Mark Cohen <[email protected]> * updated CHANGELOG.md entry removed the feature flag for pylint lint_per_folder fixed failures from mypy and pylint removed pylint MESSAGE CONTROL config from setup.cfg after relocating to lint_per_folder method Signed-off-by: Mark Cohen <[email protected]> * removed pylint ignore missing-function-docstring Signed-off-by: Mark Cohen <[email protected]> * added pylint.extensions.docparams plugin updated some docstrings to correct parameters removed pylint from setup.cfg Signed-off-by: Mark Cohen <[email protected]> * added four lints for opensearchpy/ Signed-off-by: Mark Cohen <[email protected]> * adding await back to client.info() call Signed-off-by: Mark Cohen <[email protected]> * updated TODOs as requested renamed test_opensearchpy.test_async.test_server.test_helpers.conftest.setup_ubq_tests to setup_update_by_query_tests added OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0] to skip tests list run_tests.py catches a CalledProcessError when the git repo already exists and the command to add the origin fails in fetch_opensearch_repo() Signed-off-by: Mark Cohen <[email protected]> --------- Signed-off-by: Mark Cohen <[email protected]>
2024-01-19 13:36:05 -05:00
parent 2ab3a40307
commit 0ddbf8cafa
47 changed files with 563 additions and 218 deletions
@@ -37,10 +37,16 @@ import subprocess
 import sys
 from os import environ
 from os.path import abspath, dirname, exists, join, pardir
+from subprocess import CalledProcessError
 from typing import Any


 def fetch_opensearch_repo() -> None:
+    """
+    runs a git fetch origin on configured opensearch core repo
+    :return: None if environmental variables TEST_OPENSEARCH_YAML_DIR
+    is set or TEST_OPENSEARCH_NOFETCH is set to False; else returns nothing
+    """
    # user is manually setting YAML dir, don't tamper with it
    if "TEST_OPENSEARCH_YAML_DIR" in environ:
        return
@@ -77,12 +83,20 @@ def fetch_opensearch_repo() -> None:
    # make a new blank repository in the test directory
    subprocess.check_call("cd %s && git init" % repo_path, shell=True)

-    # add a remote
-    subprocess.check_call(
-        "cd %s && git remote add origin https://github.com/opensearch-project/opensearch.git"
-        % repo_path,
-        shell=True,
-    )
+    try:
+        # add a remote
+        subprocess.check_call(
+            "cd %s && git remote add origin https://github.com/opensearch-project/opensearch.git"
+            % repo_path,
+            shell=True,
+        )
+    except CalledProcessError as e:
+        # if the run is interrupted from a previous run, it doesn't clean up, and the git add origin command
+        # errors out; this allows the test to continue
+        remote_origin_already_exists = 3
+        print(e)
+        if e.returncode != remote_origin_already_exists:
+            sys.exit(1)

    # fetch the sha commit, version from info()
    print("Fetching opensearch repo...")
@@ -90,6 +104,17 @@ def fetch_opensearch_repo() -> None:


 def run_all(argv: Any = None) -> None:
+    """
+    run all the tests given arguments and environment variables
+    - sets defaults if argv is None, running "pytest --cov=opensearchpy
+    --junitxml=<path to opensearch-py-junit.xml>
+    --log-level=DEBUG --cache-clear -vv --cov-report=<path to output code coverage"
+    * GITHUB_ACTION: fetches yaml tests if this is not in environment variables
+    * TEST_PATTERN: specify a test to run
+    * TEST_TYPE: "server" runs on TLS connection; None is unencrypted
+    * OPENSEARCH_VERSION: "SNAPSHOT" does not do anything with plugins
+    :param argv: if this is None, then the default arguments
+    """
    sys.exitfunc = lambda: sys.stderr.write("Shutting down....\n")  # type: ignore
    # fetch yaml tests anywhere that's not GitHub Actions
    if "GITHUB_ACTION" not in environ:
@@ -88,7 +88,8 @@ class TestAIOHttpConnection:
            # it means SSLContext is not available for that version of python
            # and we should skip this test.
            pytest.skip(
-                "Test test_ssl_context is skipped cause SSLContext is not available for this version of Python"
+                "Test test_ssl_context is skipped cause SSLContext is "
+                "not available for this version of Python"
            )

        con = AIOHttpConnection(use_ssl=True, ssl_context=context)
@@ -202,8 +203,8 @@ class TestAIOHttpConnection:
            con = AIOHttpConnection(use_ssl=True, verify_certs=False)
            assert 1 == len(w)
            assert (
-                "Connecting to https://localhost:9200 using SSL with verify_certs=False is insecure."
-                == str(w[0].message)
+                "Connecting to https://localhost:9200 using SSL with "
+                "verify_certs=False is insecure." == str(w[0].message)
            )

        assert con.use_ssl
@@ -379,13 +380,17 @@ class TestConnectionHttpServer:

    @classmethod
    def setup_class(cls) -> None:
-        # Start server
+        """
+        Start server
+        """
        cls.server = TestHTTPServer(port=8081)
        cls.server.start()

    @classmethod
    def teardown_class(cls) -> None:
-        # Stop server
+        """
+        stop server
+        """
        cls.server.stop()

    async def httpserver(self, conn: Any, **kwargs: Any) -> Any:
@@ -22,6 +22,10 @@ pytestmark: MarkDecorator = pytest.mark.asyncio

@fixture  # type: ignore
 async def mock_client(dummy_response: Any) -> Any:
+    """
+    yields a mock client with the dummy_response param
+    :param dummy_response: any kind of response for test
+    """
    client = Mock()
    client.search.return_value = dummy_response
    await add_connection("mock", client)
@@ -26,5 +26,6 @@ class TestPluginsClient:
            client.plugins.__init__(client)  # type: ignore
            assert (
                str(w[0].message)
-                == "Cannot load `alerting` directly to AsyncOpenSearch as it already exists. Use `AsyncOpenSearch.plugin.alerting` instead."
+                == "Cannot load `alerting` directly to AsyncOpenSearch as it already exists. Use "
+                "`AsyncOpenSearch.plugin.alerting` instead."
            )
@@ -34,13 +34,19 @@ from ...utils import wipe_cluster


 class AsyncOpenSearchTestCase(IsolatedAsyncioTestCase):  # type: ignore
-    async def asyncSetUp(self) -> None:  # pylint: disable=invalid-name
+    async def asyncSetUp(
+        self,
+    ) -> None:
+        # pylint: disable=invalid-name,missing-function-docstring
        self.client = await get_test_client(
            verify_certs=False, http_auth=("admin", "admin")
        )
        await add_connection("default", self.client)

-    async def asyncTearDown(self) -> None:  # pylint: disable=invalid-name
+    async def asyncTearDown(
+        self,
+    ) -> None:
+        # pylint: disable=invalid-name,missing-function-docstring
        wipe_cluster(self.client)
        if self.client:
            await self.client.close()
@@ -60,7 +60,9 @@ class TestYarlMissing:
    async def test_aiohttp_connection_works_without_yarl(
        self, async_client: Any, monkeypatch: Any
    ) -> None:
-        # This is a defensive test case for if aiohttp suddenly stops using yarl.
+        """
+        This is a defensive test case for if aiohttp suddenly stops using yarl.
+        """
        from opensearchpy._async import http_aiohttp

        monkeypatch.setattr(http_aiohttp, "yarl", False)
@@ -43,12 +43,24 @@ async def client() -> Any:

@fixture(scope="function")  # type: ignore
 async def opensearch_version(client: Any) -> Any:
+    """
+    yields the version of the OpenSearch cluster
+    :param client: client connection to OpenSearch
+    :return: yields major version number
+    """
    info = await client.info()
    print(info)
-    yield tuple(
-        int(x)
-        for x in re.match(r"^([0-9.]+)", info["version"]["number"]).group(1).split(".")  # type: ignore
-    )
+    yield (int(x) async for x in match_version(info))
+
+
+async def match_version(info: Any) -> Any:
+    """
+    matches the full semver server version with the given info
+    :param info: response from the OpenSearch cluster
+    """
+    match = re.match(r"^([0-9.]+)", info["version"]["number"])
+    assert match is not None
+    yield match.group(1).split(".")


@fixture  # type: ignore
@@ -60,7 +72,9 @@ async def write_client(client: Any) -> Any:

@fixture  # type: ignore
 async def data_client(client: Any) -> Any:
-    # create mappings
+    """
+    create mappings
+    """
    await create_git_index(client, "git")
    await create_flat_git_index(client, "flat-git")
    # load data
@@ -73,6 +87,11 @@ async def data_client(client: Any) -> Any:

@fixture  # type: ignore
 async def pull_request(write_client: Any) -> Any:
+    """
+    create dummy pull request instance
+    :param write_client: #todo not used
+    :return: instance of PullRequest
+    """
    await PullRequest.init()
    pr = PullRequest(
        _id=42,
@@ -96,7 +115,12 @@ async def pull_request(write_client: Any) -> Any:


@fixture  # type: ignore
-async def setup_ubq_tests(client: Any) -> str:
+async def setup_update_by_query_tests(client: Any) -> str:
+    """
+    sets up update by query tests
+    :param client:
+    :return: an index name
+    """
    index = "test-git"
    await create_git_index(client, index)
    await async_bulk(client, TEST_GIT_DATA, raise_on_error=True, refresh=True)
@@ -60,6 +60,9 @@ class FailingBulkClient(object):
        self._fail_with = fail_with

    async def bulk(self, *args: Any, **kwargs: Any) -> Any:
+        """
+        increments number of times called and, when it equals fail_at, raises self.fail_with
+        """
        self._called += 1
        if self._called in self._fail_at:
            raise self._fail_with
@@ -56,6 +56,10 @@ class MetricSearch(AsyncFacetedSearch):

@pytest.fixture(scope="function")  # type: ignore
 def commit_search_cls(opensearch_version: Any) -> Any:
+    """
+    :param opensearch_version the semver version of OpenSearch
+    :return: an AsyncFacetedSearch for git commits
+    """
    interval_kwargs = {"fixed_interval": "1d"}

    class CommitSearch(AsyncFacetedSearch):
@@ -102,6 +106,10 @@ def repo_search_cls(opensearch_version: Any) -> Any:

@pytest.fixture(scope="function")  # type: ignore
 def pr_search_cls(opensearch_version: Any) -> Any:
+    """
+    :param opensearch_version: not used here... #TODO remove this parameter?
+    :return: an AsyncFacetedSearch for pull requests
+    """
    interval_type = "calendar_interval"

    class PRSearch(AsyncFacetedSearch):
@@ -19,9 +19,9 @@ pytestmark: MarkDecorator = pytest.mark.asyncio


 async def test_update_by_query_no_script(
-    write_client: Any, setup_ubq_tests: Any
+    write_client: Any, setup_update_by_query_tests: Any
 ) -> None:
-    index = setup_ubq_tests
+    index = setup_update_by_query_tests

    ubq = (
        AsyncUpdateByQuery(using=write_client)
@@ -40,9 +40,9 @@ async def test_update_by_query_no_script(


 async def test_update_by_query_with_script(
-    write_client: Any, setup_ubq_tests: Any
+    write_client: Any, setup_update_by_query_tests: Any
 ) -> None:
-    index = setup_ubq_tests
+    index = setup_update_by_query_tests

    ubq = (
        AsyncUpdateByQuery(using=write_client)
@@ -59,9 +59,9 @@ async def test_update_by_query_with_script(


 async def test_delete_by_query_with_script(
-    write_client: Any, setup_ubq_tests: Any
+    write_client: Any, setup_update_by_query_tests: Any
 ) -> None:
-    index = setup_ubq_tests
+    index = setup_update_by_query_tests

    ubq = (
        AsyncUpdateByQuery(using=write_client)
@@ -54,6 +54,10 @@ OPENSEARCH_VERSION = None


 async def await_if_coro(x: Any) -> Any:
+    """
+    awaits if x is a coroutine
+    :return: x
+    """
    if inspect.iscoroutine(x):
        return await x
    return x
@@ -40,13 +40,15 @@ class TestSecurityPlugin(IsolatedAsyncioTestCase):  # type: ignore
    USER_NAME = "test-user"
    USER_CONTENT = {"password": "opensearchpy@123", "opendistro_security_roles": []}

-    async def asyncSetUp(self) -> None:  # pylint: disable=invalid-name
+    async def asyncSetUp(self) -> None:
+        # pylint: disable=invalid-name, missing-function-docstring
        self.client = await get_test_client(
            verify_certs=False, http_auth=("admin", "admin")
        )
        await add_connection("default", self.client)

-    async def asyncTearDown(self) -> None:  # pylint: disable=invalid-name
+    async def asyncTearDown(self) -> None:
+        # pylint disable=invalid-name
        if self.client:
            await self.client.close()

@@ -449,8 +449,10 @@ class TestTransport:
        assert event_loop.time() - 1 < t.last_sniff < event_loop.time() + 0.01

    async def test_sniff_7x_publish_host(self) -> None:
-        # Test the response shaped when a 7.x node has publish_host set
-        # and the returend data is shaped in the fqdn/ip:port format.
+        """
+        Test the response shaped when a 7.x node has publish_host set
+        and the returned data is shaped in the fqdn/ip:port format.
+        """
        t: Any = AsyncTransport(
            [{"data": CLUSTER_NODES_7X_PUBLISH_HOST}],
            connection_class=DummyConnection,
@@ -20,5 +20,6 @@ class TestPluginsClient(TestCase):
            client.plugins.__init__(client)  # type: ignore
            self.assertEqual(
                str(w.warnings[0].message),
-                "Cannot load `alerting` directly to OpenSearch as it already exists. Use `OpenSearch.plugin.alerting` instead.",
+                "Cannot load `alerting` directly to OpenSearch as "
+                "it already exists. Use `OpenSearch.plugin.alerting` instead.",
            )
@@ -139,7 +139,8 @@ class TestRequestsHttpConnection(TestCase):
            )
            self.assertEqual(1, len(w))
            self.assertEqual(
-                "Connecting to https://localhost:9200 using SSL with verify_certs=False is insecure.",
+                "Connecting to https://localhost:9200 using SSL with "
+                "verify_certs=False is insecure.",
                str(w[0].message),
            )

@@ -286,8 +287,9 @@ class TestRequestsHttpConnection(TestCase):

        # trace request
        self.assertEqual(1, tracer.info.call_count)
+        trace_curl_cmd = "curl -H 'Content-Type: application/json' -XGET 'http://localhost:9200/?pretty&param=42' -d '{\n  \"question\": \"what\\u0027s that?\"\n}'"  # pylint: disable=line-too-long
        self.assertEqual(
-            """curl -H 'Content-Type: application/json' -XGET 'http://localhost:9200/?pretty&param=42' -d '{\n  "question": "what\\u0027s that?"\n}'""",
+            trace_curl_cmd,
            tracer.info.call_args[0][0] % tracer.info.call_args[0][1:],
        )
        # trace response
@@ -415,9 +417,13 @@ class TestRequestsHttpConnection(TestCase):
        self.assertEqual('{"answer": 42}'.encode("utf-8"), request.body)

        # trace request
+        trace_curl_cmd = (
+            "curl -H 'Content-Type: application/json' -XGET 'http://localhost:9200/_search?pretty' "
+            "-d '{\n  \"answer\": 42\n}'"
+        )
        self.assertEqual(1, tracer.info.call_count)
        self.assertEqual(
-            "curl -H 'Content-Type: application/json' -XGET 'http://localhost:9200/_search?pretty' -d '{\n  \"answer\": 42\n}'",
+            trace_curl_cmd,
            tracer.info.call_args[0][0] % tracer.info.call_args[0][1:],
        )

@@ -514,7 +520,7 @@ class TestRequestsConnectionRedirect(TestCase):

    @classmethod
    def setup_class(cls) -> None:
-        # Start servers
+        """Start servers"""
        cls.server1 = TestHTTPServer(port=8080)
        cls.server1.start()
        cls.server2 = TestHTTPServer(port=8090)
@@ -522,7 +528,7 @@ class TestRequestsConnectionRedirect(TestCase):

    @classmethod
    def teardown_class(cls) -> None:
-        # Stop servers
+        """Stop servers"""
        cls.server2.stop()
        cls.server1.stop()

@@ -73,7 +73,8 @@ class TestUrllib3HttpConnection(TestCase):
            # it means SSLContext is not available for that version of python
            # and we should skip this test.
            raise SkipTest(
-                "Test test_ssl_context is skipped cause SSLContext is not available for this version of python"
+                "Test test_ssl_context is skipped cause SSLContext"
+                " is not available for this version of python"
            )

        con = Urllib3HttpConnection(use_ssl=True, ssl_context=context)
@@ -272,7 +273,8 @@ class TestUrllib3HttpConnection(TestCase):
            con = Urllib3HttpConnection(use_ssl=True, verify_certs=False)
            self.assertEqual(1, len(w))
            self.assertEqual(
-                "Connecting to https://localhost:9200 using SSL with verify_certs=False is insecure.",
+                "Connecting to https://localhost:9200 using SSL with "
+                "verify_certs=False is insecure.",
                str(w[0].message),
            )

@@ -112,7 +112,7 @@ class TestConnectionPool(TestCase):
        # Nothing should be marked dead
        self.assertEqual(0, len(pool.dead_count))

-    def test_connection_is_forcibly_resurrected_when_no_live_ones_are_availible(
+    def test_connection_is_forcibly_resurrected_when_no_live_ones_are_available(
        self,
    ) -> None:
        pool = ConnectionPool([(x, {}) for x in range(2)])
@@ -136,7 +136,12 @@ class TestParallelBulk(TestCase):

 class TestChunkActions(TestCase):
    def setup_method(self, _: Any) -> None:
-        self.actions: Any = [({"index": {}}, {"some": u"datá", "i": i}) for i in range(100)]  # fmt: skip
+        """
+        creates some documents for testing
+        """
+        self.actions: Any = [
+            ({"index": {}}, {"some": "datá", "i": i}) for i in range(100)
+        ]

    def test_expand_action(self) -> None:
        self.assertEqual(helpers.expand_action({}), ({"index": {}}, {}))
@@ -280,7 +285,9 @@ class TestScanFunction(TestCase):
    def test_scan_with_missing_hits_key(
        self, mock_search: Mock, mock_scroll: Mock, mock_clear_scroll: Mock
    ) -> None:
-        # Simulate a response where the 'hits' key is missing
+        """
+        Simulate a response where the 'hits' key is missing
+        """
        mock_search.return_value = {"_scroll_id": "dummy_scroll_id", "_shards": {}}

        mock_scroll.side_effect = [{"_scroll_id": "dummy_scroll_id", "_shards": {}}]
@@ -38,6 +38,11 @@ from opensearchpy.helpers.response.aggs import AggResponse, Bucket, BucketData

@fixture  # type: ignore
 def agg_response(aggs_search: Any, aggs_data: Any) -> Any:
+    """
+    :param aggs_search: aggregation search
+    :param aggs_data: data to aggregate
+    :return: the aggregated data
+    """
    return response.Response(aggs_search, aggs_data)


@@ -17,6 +17,9 @@ class TestHTTPRequestHandler(BaseHTTPRequestHandler):
    __test__ = False

    def do_GET(self) -> None:  # pylint: disable=invalid-name
+        """
+        writes a response out to a file given mocked parameters on this object
+        """
        headers = self.headers

        if self.path == "/redirect":
@@ -49,6 +52,9 @@ class TestHTTPServer(HTTPServer):
        self._server_thread = None

    def start(self) -> None:
+        """
+        start the test HTTP server
+        """
        if self._server_thread is not None:
            return

@@ -56,6 +62,9 @@ class TestHTTPServer(HTTPServer):
        self._server_thread.start()

    def stop(self) -> None:
+        """
+        stop the test HTTP server
+        """
        if self._server_thread is None:
            return
        self.socket.close()
@@ -53,12 +53,23 @@ def client() -> Any:

@fixture(scope="session")  # type: ignore
 def opensearch_version(client: Any) -> Any:
-    info = client.info()
+    """
+    yields a major version from the client
+    :param client: client to connect to opensearch
+    """
+    info: Any = client.info()
    print(info)
-    yield tuple(
-        int(x)
-        for x in re.match(r"^([0-9.]+)", info["version"]["number"]).group(1).split(".")  # type: ignore
-    )
+    yield (int(x) for x in match_version(info))
+
+
+def match_version(info: Any) -> Any:
+    """
+    matches the major version from the given client info
+    :param info: part of the response from OpenSearch
+    """
+    match = re.match(r"^([0-9.]+)", info["version"]["number"])
+    assert match is not None
+    yield match.group(1).split(".")


@fixture  # type: ignore
@@ -107,6 +118,7 @@ def pull_request(write_client: Any) -> Any:

@fixture  # type: ignore
 def setup_ubq_tests(client: Any) -> str:
+    # todo what's a ubq test?
    index = "test-git"
    create_git_index(client, index)
    bulk(client, TEST_GIT_DATA, raise_on_error=True, refresh=True)
@@ -509,7 +509,9 @@ class TestScan(OpenSearchTestCase):
                }
                client_mock.clear_scroll.return_value = {}

-                data = list(helpers.scan(self.client, index="test_index", **{key: val}))  # type: ignore
+                data = list(
+                    helpers.scan(self.client, index="test_index", **{key: val})  # type: ignore
+                )

                self.assertEqual(data, [{"search_data": 1}])

@@ -89,6 +89,7 @@ SKIP_TESTS = {
    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/search/aggregation/20_terms[4]",
    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/tasks/list/10_basic[0]",
    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/index/90_unsigned_long[1]",
+    "OpenSearch-main/rest-api-spec/src/main/resources/rest-api-spec/test/indices/stats/50_noop_update[0]",
    "search/aggregation/250_moving_fn[1]",
    # body: null
    "indices/simulate_index_template/10_basic[2]",
@@ -157,7 +158,7 @@ class YamlRunner:
        self._teardown_code = test_spec.pop("teardown", None)

    def setup(self) -> Any:
-        # Pull skips from individual tests to not do unnecessary setup.
+        """Pull skips from individual tests to not do unnecessary setup."""
        skip_code: Any = []
        for action in self._run_code:
            assert len(action) == 1
@@ -472,7 +473,7 @@ client = get_client()


 def load_rest_api_tests() -> None:
-    # Try loading the REST API test specs from OpenSearch core.
+    """Try loading the REST API test specs from OpenSearch core."""
    try:
        # Construct the HTTP and OpenSearch client
        http = urllib3.PoolManager(retries=10)