-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix windows integration tests. #498
Fix windows integration tests. #498
Conversation
…d functions. `eliot.log_call` uses introspection to find the argument names of a call. However: - it doesn't look through wrappers - on python 2, we can't access the wrapped generator function of a `inline_callbacks` decorated function. This introduces a wrapper around both `log_call_deferred` and `inline_callbacks` that gives `log_call_deferred` access to the original function for introspection. It also extends `log_call_deferred` to be able to log arguments, similiar to `eliot.log_call`
If a service fails before we think it has started, we would hang forever waiting for the process to start. Instead, report an error to the caller.
On windows, SIGSTOP doesn't exist, but psutil implements a way of suspending a process.
Windows uses `/scripts` instead of `/bin` for executables in a virtualenv. Expose an object representing the tahoe virtualenv that knows the correct paths.
Windows doesn't support passing file descriptors to child process, beyond stdin/stdout/stderr. Therefore, we use stderr to capture eliot logs there, making sure that other output is ignored.
Codecov Report
@@ Coverage Diff @@
## main #498 +/- ##
==========================================
- Coverage 92.02% 92.00% -0.03%
==========================================
Files 35 35
Lines 3072 3100 +28
Branches 361 369 +8
==========================================
+ Hits 2827 2852 +25
- Misses 170 173 +3
Partials 75 75
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Thanks. Some simple comments inline. Please address and then merge.
""" | ||
Like ``eliot.log_call`` but for functions which return ``Deferred``. | ||
""" | ||
|
||
if include_args: | ||
if include_args is True: |
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.
I guess this less than wonderful bool/list signature is copied from Eliot so it makes sense to keep it instead of doing something less confusing...
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.
Not exactly; eliot treats None
as include everything, instead of True
.
@@ -155,12 +163,12 @@ def tahoe_venv(request, reactor): | |||
|
|||
venv_dir = parser.get("testenv:{}".format(tahoe_env), 'envdir') | |||
|
|||
return Path(venv_dir) | |||
return VirtualEnv(Path(venv_dir)) |
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.
nice
# If we've accessed the folder state database from the integration | ||
# tests, make sure that the connection has been closed before we | ||
# try to remove the database. This is necessary on windows, | ||
# otherwise the state database can't be removed. |
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.
It seems like this might suggest that we actually shouldn't even keep a long-lived connection to the database? Perhaps there are other factors to balance this consideration against ... but "connecting" to a SQLite3 database is not a particularly expensive operations and simplifying resource management by cutting down on the number of long-lived resources to manage might be worth it.
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.
I don't think it makes sense for each test that access a folder state database to reach into MagicFolderConfig.database
and close it (I suspect that attribute should actually be private).
It isn't so much that we want to keep long-lived connections open, but that there isn't a good API for closing them. Part of the issue is we keep a cache of MagicFolderConfig
obejcts on GlobalConfigDatabase
(to ensure that we only have a single connection to each state database, and that we have access to it to close it), and there is not an API for removing an object from that cache. (See f4bbe95 for details.) I'm open to other suggestions on how to handle the interactions here.
Since FD 3 is suppposed to only have eliot-logs, log them as malformed. | ||
""" | ||
with self._action.context(): | ||
Message.log(message_type=u"malformed-eliot-log", data=data) |
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.
Can we also make this into a test-failing event so that if/when such things arise in the future we can catch them quickly? Or is there so much garbage on FD 3 that it's not even worth trying to make it clean?
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.
It is not straight-forward to do this for a couple of reasons:
- the service is not scoped to a single test
- I don't think raising an exception here would cause the test to fail, as it will be swallowed by the reactor.
I filed #510 to address this. As it stands, I don't think we should ever hit this, as FD 3 is explicitly for eliot log messages.
Fixes #503.
This might also address #484.