Port of test scripts to unit tests by hoefling · Pull Request #164 · jonashaag/bjoern

hoefling · 2019-08-01T17:17:43Z

Closes #52.

Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

jonashaag · 2019-08-02T08:28:34Z

+    @functools.wraps(f)
+    def wrapper(*args, **kwargs):
+        result = f(*args, **kwargs)
+        testclient = args[0]


better make this the first wrapper argument explicitly

jonashaag · 2019-08-02T08:28:59Z

+                exc_info = sys.exc_info()
+                exc_type = exc_info[0]
+                tb = exc_info[2]
+                self.pipe.send((exc_type, e, tb))


why not send entire exc_info

Agreed, good catch!

jonashaag · 2019-08-02T08:29:29Z

+def free_port():
+    with contextlib.closing(socket.socket(socket.AF_INET, socket.SOCK_STREAM)) as s:
+        s.bind(('', 0))
+        s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)


I guess this might break on systems that don't support reuseaddr?

Indeed, socket.REUSEADDR should be handled same as socket.REUSEPORT.

jonashaag · 2019-08-02T08:30:33Z

+            raise_(*exc_info)  # python2 compat
+
+    @reraise
+    def get(self, path=None, **kwargs):


Maybe refactor these into a single method and use functools.partial? But that's nitpicking

No, I agree on that, good suggestion. I have mainly added the method copies because of code completion in IDE, but typing stubs would be way better here.

jonashaag · 2019-08-02T08:31:19Z

+    * also, filenames longer than 64 chars cause urllib hickups, see https://bugs.python.org/issue32958
+    * another thing: tempfile.gettempdir() on MacOS resorts to $TMPDIR
+    which is autogenerated into /var/folders and is also longer than 64 chars
+    """


btw there's also anonymous sockets with @..., but not supported by all platforms. maybe we can test these as well.

Good idea! Unfortunately, it looks like urllib can't parse the URL, so requests_unixsocket can't connect - although it states the support for abstract unix sockets. I have left a comment with the error message, waiting for the author's response. Maybe extract it in a separate task?

jonashaag · 2019-08-02T08:35:15Z

+    return app
+
+
+@pytest.mark.parametrize('file', ('README.rst',), ids=('README.rst',))


Better use lists here, (foo,) is difficult to parse

jonashaag · 2019-08-02T08:35:40Z

+)
+@pytest.mark.parametrize(
+    'wrapper',
+    (


Better use [] here as well.

jonashaag · 2019-08-02T08:36:17Z

+        lambda f, env: env['wsgi.file_wrapper'](f),
+        lambda f, env: env['wsgi.file_wrapper'](f, 1),
+    ),
+    ids=('callable-iterator', 'xreadlines', 'filewrapper', 'filewrapper2'),


And here and in any place where an actual list of things is provided ;)

jonashaag · 2019-08-02T08:38:33Z

+
+
+@pytest.mark.parametrize(
+    'expect_header_value,content_length,body,response',


This is also difficult to parse

jonashaag · 2019-08-02T08:39:02Z

+            expect_header_value, content_length, body
+        )
+        raw_client.send(request.encode('utf-8'))
+        resp = raw_client.recv(1 << 10)


Why not simply write 1024 here

This is due to code copying from tests/expect100.py. Shouldn't it even be bjoern.DEFAULT_LISTEN_BACKLOG?

The reason I did that in expect100.py (I'm the author of that specific test) is because I consider it more legible when dealing with byte counts between KB -> MB -> GB. E.g.,

KB = 1024
MB = 1048576
GB = 1073741824

vs.

KB = 1 << 10
MB = 1 << 20
GB = 1 << 30

or, if your test grows the content-length exponentially with each iteration, such as that test does here

jonashaag · 2019-08-02T08:43:48Z

This is really great. Thanks a million.

Minor suggestions in the review.

Maybe you can also add some proper test description docstrings rather than tests/fileabc.py -- at this point you know better than I what the tests do! (Single sentence should be enough!)

One thing we have to think about is that some of the tests are useful only when called 1000s of times, for example the exception tests, which are used to check for memory leaks. Not sure if that's easily automated; otherwise there should be a way to run a list of WSGI applications without the actual test code so we can use the "old" way of bombarding bjoern using make ab etc

Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

hoefling · 2019-08-02T16:45:47Z

One thing we have to think about is that some of the tests are useful only when called 1000s of times, for example the exception tests, which are used to check for memory leaks.

I will look into it, however I'm not sure whether it is a task for unit tests and pytest. Does it look more like load testing? Usually I write load tests with locust, would it be an option for you?

…ded where applicable Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

jonashaag · 2019-08-21T07:42:53Z

I will look into it, however I'm not sure whether it is a task for unit tests and pytest. Does it look more like load testing? Usually I write load tests with locust, would it be an option for you?

Sorry for the delay!

Locust is fine for me, or any other tool pretty much.

Yes it's more like load testing. Best case we can load test a bjoern instance and monitor memory use

jonashaag · 2019-08-21T07:43:27Z

BUT some of the tests I also used for manual load/performance testing, for example when comparing requests/s for two revisions of bjoern. So if we could have a way to start an instance with a single test case WSGI app that would be awesome

gcavalcante8808 · 2019-11-05T23:16:35Z

Is there any plans to merge this PR? Its great for helping development.

jonashaag · 2019-11-05T23:30:35Z

Yes when the things I mentioned above are implemented.

mamico · 2019-11-07T04:54:07Z

I think that an idea could be to merge this PR and add an issue (improvement) for missing tests, probably, if @hoefling are currently busy, other contributors could help on. @jonashaag what do you think about?

jonashaag · 2019-11-07T05:14:58Z

No. That does not solve the problem that nobody is working on this PR at the moment. The current version is strictly less valuable than the one in the repo.

hoefling · 2019-11-12T23:02:45Z

This PR (and the other one I have started working at for #121) were parts of making bjoern pass the requirements of our internal QA & security team. Since then, we have moved from WSGI, so bjoern is not a requirement anymore; I thus don't have the time to work on the tests anymore. Feel free to use the code in this PR if you want to finish the task - just create your own PR and apply the diff from this one.

However, I don't see any issues with merging the current state as IIRC the existing test scripts are not removed so they are still usable for manual load tests.

jonashaag · 2019-11-13T09:10:36Z

You have a policy that 3rd party application test suites must be automated? 😀

ported test scripts to unit tests (jonashaag#52)

d95ad92

Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

hoefling mentioned this pull request Aug 1, 2019

Turn as many tests into unittests as possible #52

Open

jonashaag reviewed Aug 2, 2019

View reviewed changes

first code review

bdbbdbf

Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

hoefling force-pushed the unittests-squashed branch from 3252372 to c0006eb Compare August 3, 2019 19:23

added docstrings for tests, grouped similar tests, more parameters ad…

5f4d11d

…ded where applicable Signed-off-by: Oleg Höfling <oleg.hoefling@gmail.com>

hoefling force-pushed the unittests-squashed branch from c0006eb to 5f4d11d Compare August 3, 2019 19:28

		return app


		@pytest.mark.parametrize('file', ('README.rst',), ids=('README.rst',))



		@pytest.mark.parametrize(
		'expect_header_value,content_length,body,response',

Conversation

hoefling commented Aug 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonashaag commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoefling commented Aug 2, 2019

Uh oh!

jonashaag commented Aug 21, 2019

Uh oh!

jonashaag commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcavalcante8808 commented Nov 5, 2019

Uh oh!

jonashaag commented Nov 5, 2019

Uh oh!

mamico commented Nov 7, 2019

Uh oh!

jonashaag commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoefling commented Nov 12, 2019

Uh oh!

jonashaag commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonashaag commented Aug 2, 2019 •

edited

Loading

jonashaag commented Aug 21, 2019 •

edited

Loading

jonashaag commented Nov 7, 2019 •

edited

Loading

jonashaag commented Nov 13, 2019 •

edited

Loading