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

Port of test scripts to unit tests #164

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

Conversation

hoefling
Copy link

@hoefling hoefling commented Aug 1, 2019

Closes #52.

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

Choose a reason for hiding this comment

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

better make this the first wrapper argument explicitly

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

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

Choose a reason for hiding this comment

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

why not send entire exc_info

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, good catch!

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)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

raise_(*exc_info) # python2 compat

@reraise
def get(self, path=None, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

* 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
"""
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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?

return app


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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Agreed!

)
@pytest.mark.parametrize(
'wrapper',
(
Copy link
Owner

Choose a reason for hiding this comment

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

Better use [] here as well.

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

Choose a reason for hiding this comment

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

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



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

Choose a reason for hiding this comment

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

This is also difficult to parse

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

Choose a reason for hiding this comment

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

Why not simply write 1024 here

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Owner

jonashaag commented Aug 2, 2019

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 <[email protected]>
@hoefling
Copy link
Author

hoefling commented Aug 2, 2019

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?

@jonashaag
Copy link
Owner

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
Copy link
Owner

jonashaag commented Aug 21, 2019

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
Copy link

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

@jonashaag
Copy link
Owner

Yes when the things I mentioned above are implemented.

@mamico
Copy link

mamico commented Nov 7, 2019

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
Copy link
Owner

jonashaag commented Nov 7, 2019

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
Copy link
Author

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
Copy link
Owner

jonashaag commented Nov 13, 2019

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

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.

Turn as many tests into unittests as possible
5 participants