-
Notifications
You must be signed in to change notification settings - Fork 43
Make tests compatible with windows #252
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
Changes from all commits
d99a0a3
b1f4f28
b2c603f
f799236
e5e3c4d
f6482ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| build: false | ||
|
|
||
| platform: | ||
| - x64 | ||
|
|
||
| environment: | ||
| matrix: | ||
| - PYTHON_VERSION: 3.6 | ||
| MINICONDA_DIRNAME: C:\Miniconda36-x64 | ||
|
|
||
| - PYTHON_VERSION: 3.5 | ||
| MINICONDA_DIRNAME: C:\Miniconda35-x64 | ||
|
|
||
| - PYTHON_VERSION: 3.4 | ||
| MINICONDA_DIRNAME: C:\Miniconda34-x64 | ||
|
|
||
| matrix: | ||
| fast_finish: false | ||
|
|
||
| init: | ||
| - ECHO %PYTHON_VERSION% %MINICONDA_DIRNAME% | ||
|
|
||
| install: | ||
| - cmd: set "PATH=%MINICONDA_DIRNAME%;%MINICONDA_DIRNAME%\\Scripts;%PATH%" | ||
| - cmd: conda config --set always_yes true | ||
| - cmd: conda update --quiet conda | ||
| - cmd: conda info --all | ||
| - cmd: conda create --quiet --name conda-env-%PYTHON_VERSION% python=%PYTHON_VERSION% | ||
| - cmd: activate conda-env-%PYTHON_VERSION% | ||
| - cmd: pip install --disable-pip-version-check --user --upgrade pip | ||
| - cmd: pip install -e . | ||
| - cmd: pip install pytest pytest-xdist | ||
|
|
||
| test_script: | ||
| - cmd: pytest --color=yes -n 2 -sv |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,12 +28,16 @@ | |
| from common import append_received | ||
| from common import set_received | ||
|
|
||
| from common import skip_windows_spawn | ||
| from common import skip_windows_any_port | ||
| from common import skip_windows_port_reuse | ||
|
|
||
|
|
||
| def test_agent_uuid(): | ||
| """ | ||
| All agent identifiers should be unique strings. | ||
| """ | ||
| N = 1000 | ||
| N = 100 | ||
| bunch = set(Agent()._uuid for i in range(N)) | ||
| assert len(bunch) == N | ||
| assert all(isinstance(identifier, bytes) for identifier in bunch) | ||
|
|
@@ -265,6 +269,7 @@ def test_bind_tcp_addr_specific_port(nsproxy): | |
| assert address.address.port == port | ||
|
|
||
|
|
||
| @skip_windows_spawn | ||
| @pytest.mark.parametrize('linger, sleep_time, should_receive', [ | ||
| (2, 1, True), | ||
| (0.5, 1, False), | ||
|
|
@@ -301,7 +306,7 @@ def on_init(self): | |
| puller.bind('PULL', alias='pull', handler=append_received, | ||
| addr=address.address, transport='tcp') | ||
|
|
||
| assert should_receive == wait_agent_attr(puller, data='foo', timeout=.2) | ||
| assert should_receive == wait_agent_attr(puller, data='foo', timeout=1) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this timeout does not need to be changed for now as this test is skipped for Windows. When/if we remove the skip we will change the timeout if necessary.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But this one does fail on Linux from time to time. I ran it in a loop yesterday, and it took me anywhere between 50 and 500 attempts to make it fail, but eventually it does. It has failed a few times in travis: I think 1 is a safer value.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ocaballeror Then ok. It seems changing the timeout does not actually change the test, will only make it a bit slower for the cases where Should we put it as a separate commit? i.e.: "More lenient timeout in linger test". Simply because it is not really related to Windows tests.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense 👍 Rebasing this is going to be fun.... |
||
|
|
||
|
|
||
| def test_pushpull(nsproxy): | ||
|
|
@@ -599,6 +604,7 @@ def test_running_exception(nsproxy): | |
| assert not agent.is_running() | ||
|
|
||
|
|
||
| @skip_windows_port_reuse | ||
| def test_agent_error_address_already_in_use(nsproxy): | ||
| """ | ||
| Running an agent should raise an error if address is already in use. | ||
|
|
@@ -609,6 +615,7 @@ def test_agent_error_address_already_in_use(nsproxy): | |
| assert 'Address already in use' in str(error.value) | ||
|
|
||
|
|
||
| @skip_windows_any_port | ||
| def test_agent_error_permission_denied(nsproxy): | ||
| """ | ||
| Running an agent should raise an error if it has not sufficient | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,7 +57,7 @@ def test_wrong_nameserver_address(timeout): | |
| t0 = time.time() | ||
| with pytest.raises(TimeoutError): | ||
| locate_ns('127.0.0.1:1234', timeout=timeout) | ||
| assert timeout <= time.time() - t0 <= timeout + 1. | ||
| assert timeout <= time.time() - t0 <= timeout + 1.2 | ||
|
|
||
|
|
||
| def test_no_timeout_locate_ns_existing(nsproxy): | ||
|
|
@@ -323,4 +323,4 @@ def shoot(self): | |
| assert time.time() - t0 < 0.2 | ||
| assert not wayne._next_oneway | ||
|
|
||
| assert wait_agent_attr(wayne, value=20*['bang!'], timeout=1.2) | ||
| assert wait_agent_attr(wayne, value=20*['bang!'], timeout=1.5) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this failing with the previous timeout?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe at some point, I wouldn't have changed it otherwise. I'll run it through appveyor again, maybe it needed longer timeouts before, since we were using 8 threads instead of 2.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It still fails if set back to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, I ran it in a loop to see if it would eventually fail, and after 100-200 tests I've gotten it to fail a couple of times. 1.5 looks like a safer value. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Is it too slow on Windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I don't understand why, but for some reason, it takes 12-13 seconds to complete on Windows and about 0.2 seconds on Linux. I thought
N=100would be enough for this test anywayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew Windows processes were slow, but did not know they were that slow. The main difference is the way they are created (spawn vs. fork). Forking is definitely much faster, specially with the copy-on-write implemented in Linux.
Leave it with 100 then. 👍