Tests improvements #40

pdyba · 2021-08-18T08:42:15Z

No description provided.

…mypy � Conflicts: � lbz/authz.py � lbz/exceptions.py � version

…tests_improvements � Conflicts: � CHANGELOG.md � Makefile � lbz/authentication.py � lbz/authz.py � lbz/dev/misc.py � lbz/dev/server.py � lbz/dev/test.py � lbz/jwt_utils.py � lbz/misc.py � lbz/request.py � lbz/resource.py � lbz/router.py � setup.cfg � tests/fixtures/cognito_auth.py � tests/fixtures/rsa_pair.py � tests/test_authz_authorizer.py � tests/test_authz_decorator.py � tests/test_dev_misc.py � tests/test_dev_server.py � tests/test_resource.py � version

…tests_improvements � Conflicts: � CHANGELOG.md � lbz/authz/authorizer.py � version

sonarqubecloud · 2021-08-27T14:48:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

No Coverage information
0.0% Duplication

redlickigrzegorz · 2021-08-29T14:19:12Z

CHANGELOG.md

+Released 2021-08-30
+
+- Code quality improvements, brings coverage to 99%
+- Adds more functional test for DevServer


Typo: "... more functional tests ..." - plural form.

redlickigrzegorz · 2021-08-29T14:19:52Z

CHANGELOG.md

+
+- Code quality improvements, brings coverage to 99%
+- Adds more functional test for DevServer
+- Improves tests exectuion time (15s -> <5s)


Typo: "execution"

redlickigrzegorz · 2021-08-29T19:35:54Z

Makefile

+	coverage report --skip-covered -m
+
+real-coverage:
+	for file in $$(find lbz -type f \( -name "*.py" -and ! -name "*_.py" -and ! -name "event.py" \)); do       \


Did you take that from the internet or did you prepare that by yourself? 🤔

I am asking because this loop is not that good for the future. At least I see two problems here:

You skipped here all the files ended by an underscore:

sometimes there is a logic in __init__.py files and then most likely it is tested in the module which is named according to the package

also, there might be a module with a name ending with an underscore, just to avoid the conflict with the installed library e.g. boto3_.py or requests_.py

You forced the name of the test module and this rule is not written down anywhere (or maybe I haven't seen that yet) - that gives too little freedom in programming.

I think features like this should be re-thought and introduced separately (not in a pull request which has more than 1k lines added).

But if you want to keep it here, it should at least support splitting tests into the packages:

├── test_<package-1> │ ├── __init__.py │ ├── test_<module-1>.py │ ├── test_<module-2>.py │ ├── ...

Keep in mind that it will probably extend the time of reviewing/closing this pull request 😉

its my own, its not a feature its supporting script. We can skip it for now.
There orginal rule was to name tests test_module_its_testing.py.
Its not in the pipeline so we can improve that further in the future.
Spliting the test in packages makes sense but the makefile script is a PITA.

Ok, so just remove that or comment out with putting a nice TODO and place the test modules in the proper packages (we have two at this moment: authz and dev) 😉 We will return to that with a smaller PR.

redlickigrzegorz · 2021-08-29T19:50:03Z

lbz/jwt_utils.py

@@ -3,7 +3,7 @@
 JWT helpers module.
 """
 import json
-import os
+from os import environ as env


I don't think renaming the built-in variable is a good idea. That is pretty well-known over the whole world and introducing env may suggest that here, something different is used for environment variables.

Also, you are not consistent in this change, so I would leave that exactly as it was implemented before 😉

lbz/jwt_utils.py

redlickigrzegorz · 2021-08-29T20:39:33Z

lbz/dev/server.py

@@ -24,19 +26,21 @@ class MyLambdaDevHandler(BaseHTTPRequestHandler, metaclass=ABCMeta):
    @property
    @abstractmethod
    def cls(self) -> Type[Resource]:
-        pass
+        """
+        DevServer needs to inherit a Resource.


This is a misleading sentence as the current pattern does not rely on inheriting: resource is just assigned to cls on the class declaration level.

In the past, I wanted to remove this cls abstract method at all, or at least write a proper TODO because things like this need to be done simpler (developer-friendly).

redlickigrzegorz · 2021-08-29T20:41:47Z

lbz/dev/server.py

@@ -56,9 +60,9 @@ def _get_route_params(self, org_path: str) -> Tuple[Union[str, None], Union[dict
                        acc += 1
                if len(path) == acc:
                    return org_route, params
-        return None, None
+        raise ValueError


Please place here an error message to say directly what happened - it will simplify resolving problems in the future 😉

redlickigrzegorz · 2021-08-29T20:49:25Z

lbz/dev/server.py


-    def _send_json(self, code: int, obj: dict, headers: dict = None) -> None:
+    def _send_json(self, code: int, obj: Union[str, dict], headers: dict = None) -> None:


As I see, obj is only used at the end of this method, to be JSON dumped. But if it is already a string, that line will produce a different result - I am not sure if it is something that should be accepted, at least it doesn't look so:

>>> json.dumps({"data": []}, indent=4, sort_keys=True) '{\n "data": []\n}' >>> json.dumps('{"data": []}', indent=4, sort_keys=True) '"{\\"data\\": []}"'

redlickigrzegorz · 2021-08-29T20:54:47Z

lbz/dev/server.py

@@ -110,15 +110,8 @@ def handle_request(self) -> None:
                    body=request_obj,
                )
            )
-            response = resource()
-            code = response.status_code
-            response_as_dict = response.to_dict()


There is a piece of logic inside the .to_dict() method which makes sense to me but you skipped it and you always returns raw body:

body = ( json.dumps(self.body, separators=(",", ":"), default=str) if self.is_json else self.body )

What do you think about that? 🤔

redlickigrzegorz · 2021-08-29T20:59:22Z

lbz/dev/server.py

@@ -142,7 +135,7 @@ def do_OPTIONS(self) -> None:  # pylint: disable=invalid-name
        self.handle_request()


-class MyDevServer:
+class MyDevServer(Thread):


Can you describe in a few words what gives us this inheritance? I can investigate it on my own but I thought it will be simpler/faster to get it from you 😉

redlickigrzegorz · 2021-09-03T11:08:59Z

tests/conftest.py


+req = Request(


The previous way how this fixture was built was correct. You shouldn't declare objects like this globally because if it changes, it will affect all the tests that are using it - bugs like that are very hard to find.

Request in consider unmutable (except methods but they are not changing the data)

Unfortunately, there are places where this rule is broken e.g. test_resource.py. By using fixtures, we have a much clearer situation because a new instance will be created for each test 😉

tests/conftest.py

redlickigrzegorz · 2021-09-03T11:11:25Z

tests/conftest.py


+@pytest.fixture(autouse=True)


autouse=True means that this fixture will be used automatically for every test. As not all of them are using sample_request, I would say, it is too much 😉

redlickigrzegorz · 2021-09-03T11:11:52Z

tests/conftest.py

-@pytest.fixture
-def sample_event():
+
+@pytest.fixture(autouse=True)


ditto: remove autouse=True

redlickigrzegorz · 2021-09-03T11:14:54Z

tests/conftest.py

    return Event(
        resource_path="/",
        method="GET",
        headers={},
        path_params={},
        query_params={},
+        body=req,


You put here a Request but dict is expected. Sad that mypy is still disabled for tests, it should catch that automatically.

redlickigrzegorz · 2021-09-03T11:24:59Z

tests/conftest.py

+
+
+@pytest.fixture(autouse=True)
+def user() -> User:


All the above fixtures that are built based on global variables may behave differently depending on the usage of the whole code. In my opinion, you should stick to the simple rule: "if you need a global variable, declare a global variable but if you want to have a fixture instead, put the logic in the fixture's body" 😉

Personally, I recommend using fixtures (fewer problems) but definitely, we shouldn't mix those solutions.

Actually in orded to makes test faster we need to fo all jwt related, but we might store them in more "private way"

As we agreed offline, the pytest fixtures with the scope set to session might be the solution in this situation.

redlickigrzegorz · 2021-09-03T11:32:14Z

tests/conftest.py

+}
+TOKEN = encode_token(COGNITO_USER)
+
+with patch("lbz.jwt_utils.PUBLIC_KEYS", [SAMPLE_PUBLIC_KEY]), patch(


Patching things globally makes sense only if the order of creating objects matters. Here, just a User is created, so, it can be easily enclosed in the fixture too 😉

redlickigrzegorz · 2021-09-03T11:40:55Z

tests/sample_test_resources.py

+from lbz.router import add_route
+
+
+class SimpleTestResource(Resource):


It looks like those classes are used only for testing the dev package, am I right?

I will review them based on the test cases but please put it under the test_dev package, it will be simpler to see the dependencies in this code thanks to that 😉 It can be placed even in the test_dev/conftest.py module following the pattern.

redlickigrzegorz · 2021-09-03T11:45:43Z

tests/utils.py

+    return encodeded_jwt
+
+
+allowed_audiences = [str(uuid4()), str(uuid4())]


As a global variable, it shouldn't be placed in the utils.py module, and also, it should be named with uppercase.

I also started thinking about generating those all unique identifiers 🤔 I see that uuid4() is used often to declare global variables at the top level.

Do we have to do this? Globally? For all the tests? Is it not true that it is needed only for a part of them and the rest can work pretty well with hardcoded values? I would like to introduce some kind of separation here to simplify maintenance in the future 😉

redlickigrzegorz · 2021-09-03T11:57:43Z

tests/conftest.py

+
+AUTH_HEADER = Authorizer.sign_authz(
+    {**_base_auth_payload(), "allow": {"test_res": {"perm-name": {"allow": "*"}}}, "deny": {}},
+    SAMPLE_PRIVATE_KEY,


Part of the things is implemented here, part of them in other modules and part under the fixtures package. We should think about placing them in the right place based on the usage:

lbz/tests ├── test_authz │ ├── __init__.py │ ├── conftest.py # all fixtures used for testing authz │ ├── ... ├── test_dev │ ├── __init__.py │ ├── conftest.py # all fixtures used for testing dev │ ├── ... ├── __init__.py ├── conftest.py # all fixtures used by other tests ├── ...

We can also think about having constants.py and utils.py but only if really needed 😉

redlickigrzegorz · 2021-09-03T12:09:11Z

tests/test_router.py

        # pylint: disable= attribute-defined-outside-init
        self.router = Router()
        self.router.add_route("/", "GET", "x")

-    def teardown_method(self, _test_method):
+    def teardown_method(self, _test_method: Callable) -> None:


I think you could use types.MethodType here instead 😉

Side note: we don't have mypy enabled for tests, so they are not checked automatically - introducing types into this big pull request may make a review longer 😐

redlickigrzegorz · 2021-09-03T12:12:47Z

tests/test_response.py

@@ -67,3 +69,7 @@ def test_response_body_passes_through_when_base64(self):
            "statusCode": 666,
            "isBase64Encoded": True,
        }
+
+    def test_unsported_header_raises(self) -> None:


typo: "unsupported"

It would be good to place a full sentence too, including the reason - it will be valuable for future developers.

redlickigrzegorz · 2021-09-03T12:14:38Z

tests/test_response.py

+
+    def test_unsported_header_raises(self) -> None:
+        with pytest.raises(RuntimeError):
+            Response(b"xxx", status_code=666)


The name of the test is suggesting that the header is tested but here you put body and the status_code. So, what exactly is tested here? 🤔

Also, the body expects to get str or dict but you provided bytes - mypy would catch that, I guess.

redlickigrzegorz · 2021-09-03T12:22:55Z

tests/test_request.py

@@ -37,19 +34,10 @@ def test__init__(self):


 class TestRequest:
-    def setup_method(self):
+    @pytest.fixture(autouse=True)


You shouldn't mark setup_method as a fixture - that is a wrong usage of this functionality. You should prepare a normal fixture above which will be used in the tests:

@pytest.fixture def sample_request(user: User) -> Request: return Request( ... user=user, )

Or, if you want to keep the setup_method as it is right now, you should prepare a factory that can be used within this method. But it will be rather a normal function - it won't break anything in the pytest fixtures 😉

redlickigrzegorz · 2021-09-03T12:29:41Z

tests/test_misc.py

    nest = NestedDict()
    nest["a"]["b"]["c"]["d"]["e"] = "z"
    assert nest == {"a": {"b": {"c": {"d": {"e": "z"}}}}}
+    assert nest["a"]["b"]["c"]["d"]["e"] == "z"


If the above is true, this line just tests that Python's dict is working correctly 😉

I would say, it is not needed but it is just a thought.

redlickigrzegorz · 2021-09-03T12:38:23Z

tests/test_misc.py

+    assert multi_dict._dict == {}  # pylint: disable=protected-access
+
+
+def test_multi_dict_index_error() -> None:


Based on the test implementation, I think the name should be improved, something like:

def test__multi_dict__raises_key_error_if_value_of_key_is_empty_list() -> None

On the other hand, I have to say that this is a bit counterintuitive and I would treat it as a bug but maybe I am wrong - documentation may help here a little 😉

redlickigrzegorz · 2021-09-03T12:40:55Z

tests/test_jwt_utils_import_error.py

+import pytest
+
+
+def not_working_test() -> None:  # TODO: finish me! - lbz/jwt_utils.py:26 mocking hell


Please use @pytest.mark.skip to skip tests that you don't want to run. It will be at least shown in the report 😉 Also, it would be good to write a more descriptive TODO, at the first look, I don't have a clue what you wanted to achieve here.

redlickigrzegorz · 2021-09-03T12:45:27Z

tests/test_authentication.py



 @patch("lbz.jwt_utils.PUBLIC_KEYS", [SAMPLE_PUBLIC_KEY])
 @patch("lbz.jwt_utils.ALLOWED_AUDIENCES", allowed_audiences)
 class TestAuthentication:
-    def setup_class(self):
-        # pylint: disable=attribute-defined-outside-init
+    @pytest.fixture(autouse=True)


ditto: wrong usage of pytest.fixture

redlickigrzegorz · 2021-09-03T12:47:50Z

tests/test_authentication.py

-        sample_user = User(encode_token({"cognito:username": username}))
-        assert sample_user.__repr__() == f"User username={username}"
+        sample_user_2 = User(encode_token({"type": "x"}))
+        assert sample_user_2.__repr__() == "User"


Please put these assertions into separate tests - they represent two cases.

redlickigrzegorz · 2021-09-03T12:49:01Z

tests/test_authentication.py

        with pytest.raises(Unauthorized), patch(
            "lbz.jwt_utils.PUBLIC_KEYS",
            [{**SAMPLE_PUBLIC_KEY.copy(), "n": str(uuid4())}],
        ):
            User(self.id_token)

-    def test_loading_user_parses_user_attributes(self):
-        parsed = self.cognito_user.copy()
+    def test_loading_user_parses_user_attributes(self, user_cogniot) -> None:


typo: "user_cognito"

redlickigrzegorz · 2021-09-06T15:22:47Z

tests/test_resource.py

-class TestResource:
-    def setup_method(self):
+class TestResourceInit:
+    @pytest.fixture(autouse=True)


ditto: wrong usage of pytest fixtures.

redlickigrzegorz · 2021-09-06T15:27:22Z

tests/test_resource.py

-
-class TestResource:
-    def setup_method(self):
+class TestResourceInit:


Why did you split the tests of Resource into two classes? 🤔 As for me, having them together makes sense.

redlickigrzegorz · 2021-09-06T15:30:08Z

tests/test_resource.py

-    #         "guest_permissions": {},
-    #     }
-    # }
+    def test___repr__(self) -> None:


You put here too many underscores. I know the method contains its own underscores but in general, putting them into the name of the test makes them barely visible (underscores means word separation in the tests) 😉

redlickigrzegorz · 2021-09-06T15:32:43Z

tests/test_resource.py

-    # }
+    def test___repr__(self) -> None:
+        self.res.urn = "/foo/id-12345/bar"
+        assert str(self.res) == "<Resource GET @ /foo/id-12345/bar >"


If you test the __repr__() method of this class, you should use the built-in repr() function 😉

str() depends on both __str__() and __repr__().

redlickigrzegorz · 2021-09-06T15:34:33Z

tests/test_resource.py

        class XResource(Resource):
            @add_route("/")
-            def test_method(self):
+            def test_method(self) -> None:


This method returns the Response instance, not None.

redlickigrzegorz · 2021-09-06T16:30:03Z

tests/test_resource.py

@@ -330,6 +317,15 @@ def test_options_request(self):
            ALLOW_ORIGIN_HEADER: ORIGIN_EXAMPLE,
        }

+    @patch.object(CORSResource, "resp_headers", return_value={})
+    def test_not_allow_oring_in_header(self, resp_headers_mock: MagicMock) -> None:
+        an_event = defaultdict(MagicMock())


Why didn't you use self.make_cors_handler? It looks like it builds exactly the same thing as you implemented here 🤔

redlickigrzegorz · 2021-09-06T16:31:16Z

tests/test_resource.py

@@ -330,6 +317,15 @@ def test_options_request(self):
            ALLOW_ORIGIN_HEADER: ORIGIN_EXAMPLE,
        }

+    @patch.object(CORSResource, "resp_headers", return_value={})
+    def test_not_allow_oring_in_header(self, resp_headers_mock: MagicMock) -> None:


typo: "origin"

Please, describe it better. I think CORSResource allows having origin in headers but value can be unacceptable.

redlickigrzegorz · 2021-09-06T16:35:09Z

tests/test_resource.py

+        an_event["method"] = "GET"
+        an_event["headers"] = {"origin": "notlocalhost"}
+        resp = CORSResource(an_event, ["GET", "POST"], origins="localhost")()
+        assert resp.status_code >= 400


This assertion gives a lot of options, please, choose one status code 😉

redlickigrzegorz · 2021-09-06T16:35:38Z

tests/test_resource.py

@@ -330,6 +317,15 @@ def test_options_request(self):
            ALLOW_ORIGIN_HEADER: ORIGIN_EXAMPLE,
        }

+    @patch.object(CORSResource, "resp_headers", return_value={})


Why did you mock that? Isn't it better to check the result instead?

redlickigrzegorz · 2021-09-06T16:39:50Z

tests/test_resource.py

@@ -338,15 +334,16 @@ def setup_method(self, _test_method, _init_mock: MagicMock) -> None:
        self.resource = PaginatedCORSResource({}, [])
        self.resource.path = "/test/path"
        self.resource.urn = "/test/path"
-        req.query_params = MultiDict(
+        self.req = pytest.mark.usefixtures("sample_request")


This won't work as you expect. pytest.mark.usefixtures is a decorator, so you will get here the instance of one of pytest internal classes. Please. initialize the request normally 😉

redlickigrzegorz · 2021-09-07T12:00:02Z

tests/test_authz_utils.py

+
+        with patch("lbz.authz.authorizer.decode_jwt", lambda _: {**base_auth_payload, **acl}):
+            assert (
+                has_permission(SampleResource(sample_event), "sample_function") is expected_result


This function belongs to the authz/decorators.py module - I think, it would be good to create authz/utils.py for that if you have a test module named this way 😉

It would also resolve the naming problem of decorators which I pointed in the previous PR.

redlickigrzegorz · 2021-09-07T12:25:08Z

tests/test_authz_utils.py

+    ) -> None:
+        sample_event["headers"]["authorization"] = "dont_care"
+
+        with patch("lbz.authz.authorizer.decode_jwt", lambda _: {**base_auth_payload, **acl}):


Do you need to return base_auth_payload if decode_jwt is mocked? Why not make it simpler:

with patch("lbz.authz.authorizer.decode_jwt", return_value=acl):

redlickigrzegorz · 2021-09-07T12:26:08Z

tests/test_authz_utils.py

+        sample_event["headers"]["authorization"] = "dont_care"
+
+        with patch("lbz.authz.authorizer.decode_jwt", lambda _: {**base_auth_payload, **acl}):
+            assert (


For the readability: create a variable and then make an assertion 😉

redlickigrzegorz · 2021-09-07T12:29:02Z

tests/test_authz_decorators.py

+
+@patch.dict(environ, env_mock)
+class TestAuthorizationDecorator:
+    @pytest.fixture(autouse=True)


ditto: wrong usage of pytest fixtures.

redlickigrzegorz · 2021-09-07T12:29:49Z

tests/test_authz_decorators.py

+@patch.dict(environ, env_mock)
+class TestAuthorizationDecorator:
+    @pytest.fixture(autouse=True)
+    def setup_class(self, auth_header, sample_event) -> None:


ditto: setup_class should be a class method.

redlickigrzegorz · 2021-09-07T13:42:36Z

tests/test_authz_authorizer.py

+
+    def test_check_allow_domain(self) -> None:
+        self.authz.allow = {"test_resource": ALL}
+        self.authz._check_allow_and_set_resources()  # pylint: disable=protected-access


Why do you test a protected method? That can be used differently inside the class. I think testing check_access() makes the most sense.

becouse here we are testing the authorize module ergo all its private and magick methods.

But I am speaking about the general approach to testing - if something has been marked as protected, you shouldn't use it outside the class/module. If you need to do this, that starts smelling like a problem either in the tests or in the code generally 😬

Of course, you can simply mark that as public (not recommended) or test something that is public and what will be really used by the outside world 😉

agreed to disagree offline.

As below: we will come back to this topic in the future, because it is an important thing when designing the code 😉

tests/test_authz_authorizer.py

redlickigrzegorz · 2021-09-07T13:52:28Z

tests/test_authz_authorizer.py

+    def test__check_resource(self) -> None:
+        with patch.object(self.authz, "_deny_if_all", new_callable=MagicMock()) as mocked_deny:
+            self.authz._check_resource({"res": "xxx"})  # pylint: disable=protected-access
+            mocked_deny.assert_has_calls(


This whole test is wrongly designed:

_deny_if_all is a very very simple method, it doesn't have to be mocked.

_check_resource is a protected method - shouldn't be tested.

By checking that one protected method triggered another protected method three times, you have frozen the implementation, not the result of the code.

as above it this module it need to be checked.

It depends on what you want to achieve, right now, with these tests, there is no chance to change the code without getting errors. In general, that is ok but it blocks the code much - it won't give the possibility to easily refactor the code.

By tests, we should check everything that is important meaning e.g. the result, but it does not matter how the result has been achieved 😉

agreed to disagree offline.

Temporarily yes, but be aware I will return to this topic sooner or later 😉

tests/test_authz_authorizer.py

redlickigrzegorz

Ok, I think that is the last review that I have done for this enormous pull request. My recommendation would be to not touch this branch at all. A much simpler and quicker solution will be to split those changes into several SMALLER pull requests and merge them separately 😉

redlickigrzegorz · 2021-09-08T13:19:53Z

tests/test_jwt_utils.py

+from lbz.jwt_utils import get_matching_jwk, decode_jwt
+from tests import SAMPLE_PUBLIC_KEY, SAMPLE_PRIVATE_KEY
+
+BAD_PUBLICK_KEY = {


typo: "BAD_PUBLIC_KEY"

redlickigrzegorz · 2021-09-08T13:22:25Z

tests/test_jwt_utils.py

+from tests import SAMPLE_PUBLIC_KEY, SAMPLE_PRIVATE_KEY
+
+BAD_PUBLICK_KEY = {
+    "kid": "wrongd75-3f54-4155-84fa-c1a17da22b35",


I think simple "wrong-key" would be better than mixing it with generated-like string 😉

redlickigrzegorz · 2021-09-08T13:23:04Z

tests/test_jwt_utils.py

+    "kid": "wrongd75-3f54-4155-84fa-c1a17da22b35",
+}
+
+ALLOWED_AUDIENCES = [str(uuid4()), str(uuid4())]


I saw it in utils.py before, it would be good to declare it once and reuse it then 😉

redlickigrzegorz · 2021-09-08T13:33:25Z

tests/test_jwt_utils.py

+            get_matching_jwk("x")
+
+
+@patch("lbz.jwt_utils.PUBLIC_KEYS", [SAMPLE_PUBLIC_KEY])


Those variables bases on the values which are taken directly from the environment variables declared in the __init__.py module - I don't think it is needed to be set here unless you plan to remove mentioned declaration, do you?

redlickigrzegorz · 2021-09-08T13:35:27Z

tests/test_jwt_utils.py

+@patch("lbz.jwt_utils.PUBLIC_KEYS", [SAMPLE_PUBLIC_KEY])
+@patch("lbz.jwt_utils.ALLOWED_AUDIENCES", ALLOWED_AUDIENCES)
+class TestDecodeJWT:
+    @patch("lbz.jwt_utils.get_matching_jwk", return_value={})


Based on the implementation and above tests, it looks like it is not possible to get an empty dict from the get_matching_jwk() function.

redlickigrzegorz · 2021-09-08T15:08:16Z

tests/test_dev_server.py

+    assert params is None
+
+    with pytest.raises(ValueError):
+        handler._get_route_params("")  # pylint: disable=protected-access


Keep the test cases separately.

redlickigrzegorz · 2021-09-08T15:15:39Z

tests/test_dev_server.py

+def test_handle_request(method) -> None:
+    with patch("socket.socket") as msocket:
+        msocket.makefile = lambda a, b: io.BytesIO(b"GET / HTTP/1.1\r\n")
+        msocket.rfile.close = lambda: 0


The msocket variable should become a separate fixture. Mostly, to describe that better but also, to reduce the duplication. The biggest value of the tests is the documentation which can be checked anytime by anyone - so, we need to try to make the tests simple 😉

redlickigrzegorz · 2021-09-08T15:29:05Z

tests/test_dev_server.py

+def test_my_dev_server_run() -> None:
+    dev_serv = MyDevServer(MyLambdaDevHandlerHelloWorld)
+    dev_serv.start()
+    assert dev_serv.is_alive()


Don't you test that in a separate test module? test_dev_server_running? 🤔 I think I am getting a little lost. You are mixing the running server with the one which is partially mocked, and with that poor naming of the tests and duplicate part of the code, this won't be maintainable.

I wrote it before, but part of the tests for this library is more like integration tests which should be kept separately and better implemented/documented.

redlickigrzegorz · 2021-09-08T15:34:14Z

tests/test_dev_server.py

+            msocket.rfile.close = lambda: 0
+            MyLambdaDevHandlerHelloWorld(msocket, ("127.0.0.1", 8888), BaseServer)
+
+        mocked_send.assert_has_calls(


In both test_handle_txt and test_handle_json, you are checking if one of the mocked/protected methods was triggered according to the needs. In general, that is not what the test should test 😐 You should rather compare the results which you are getting.

redlickigrzegorz · 2021-09-08T15:36:38Z

tests/test_dev_misc.py


-def hash_string(string):
+
+def hash_string(string) -> None:


This function does not return None.

pdyba added 24 commits July 23, 2021 18:07

adds mypy and flake8

8f05fc6

updates version

52d2341

updates makefile

ceca524

changes from code review and new mypy config

4ffb5e8

adjusts makefile

4b0e500

Merge branch 'master' of https://github.com/pdyba/lambdalizator into …

0863aea

…mypy � Conflicts: � lbz/authz.py � lbz/exceptions.py � version

changes from code review

a2c3152

test improvements

23d0e82

test improvements

6715edc

brings coverage to 99% - only one statement missing

52bad22

liny, black mypy fixes

b23d240

merge with master

d8a233b

fixes requrirements

4fa24ad

Merge branch 'master' of https://github.com/pdyba/lambdalizator into …

621d38e

…tests_improvements � Conflicts: � CHANGELOG.md � lbz/authz/authorizer.py � version

merge with master

ced16d4

adds better coverage measuring

19ad38d

futher improvements

2d2e6d8

futher improvements

cdd0b9c

fixes jwt test

655e7f8

profiled test final

e2ba69a

linting fixes

e4f9092

blacked

1041796

blacked

5405408

redlickigrzegorz reviewed Aug 29, 2021

View reviewed changes

redlickigrzegorz reviewed Sep 3, 2021

View reviewed changes

redlickigrzegorz reviewed Sep 6, 2021

View reviewed changes

redlickigrzegorz reviewed Sep 7, 2021

View reviewed changes

redlickigrzegorz reviewed Sep 8, 2021

View reviewed changes

redlickigrzegorz mentioned this pull request Sep 12, 2021

0.3.16 stable #43

Merged


		def _send_json(self, code: int, obj: dict, headers: dict = None) -> None:
		def _send_json(self, code: int, obj: Union[str, dict], headers: dict = None) -> None:

		from lbz.router import add_route


		class SimpleTestResource(Resource):

		return encodeded_jwt


		allowed_audiences = [str(uuid4()), str(uuid4())]

		assert multi_dict._dict == {} # pylint: disable=protected-access


		def test_multi_dict_index_error() -> None:

		import pytest


		def not_working_test() -> None: # TODO: finish me! - lbz/jwt_utils.py:26 mocking hell

		get_matching_jwk("x")


		@patch("lbz.jwt_utils.PUBLIC_KEYS", [SAMPLE_PUBLIC_KEY])


		req = Request(

Tests improvements #40

Are you sure you want to change the base?

Tests improvements #40

Conversation

pdyba commented Aug 18, 2021

sonarqubecloud bot commented Aug 27, 2021

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

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

redlickigrzegorz Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redlickigrzegorz Sep 11, 2021 • 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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redlickigrzegorz Sep 11, 2021 • 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

redlickigrzegorz Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redlickigrzegorz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redlickigrzegorz Sep 3, 2021 •

edited

Loading

redlickigrzegorz Sep 11, 2021 •

edited

Loading

redlickigrzegorz Sep 11, 2021 •

edited

Loading

redlickigrzegorz Sep 11, 2021 •

edited

Loading