-
Notifications
You must be signed in to change notification settings - Fork 275
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
Log subproceses stdout and stderr in temp files #1104
Log subproceses stdout and stderr in temp files #1104
Conversation
Two comments from a quick test:
|
Yes, I checked that and you are right. The solution that I propose is to use two temporary files: one for stdout and one for stderr.
If we separate stderr from stdout as I proposed above, then we can use My bigger concern with that approach is how we are going to propagate the cmd option when using tox or even calling the tests with |
You can do that but I don't think it's necessarily worth the effort:
I think dumping all the server output to debug or info is fine |
You know what: we should think about combining (or re-ordering) this PR and issue #1111 . If we had this in utils.py (edited after chat with Martin):
I think this PR could be vastly smaller and easier to modify later, AND we would have the infra needed by #1111 |
I am working to create an abstraction that will handle the subprocess and the temporary files used for logging. |
94291b6
to
e7f56d0
Compare
I have pushed the initial set of commits in which I created the class which handles the server subprocess and the temporary files used for logging. I have decided to use On a successful test right now no logs like I am planning to push at least 1 commit more in which I will remove or reword some of the test comments. |
e7f56d0
to
b35a536
Compare
Looks good, thanks! I'll take this for a test run later to see what the output looks like |
I don't know why but when I run Same for every file where I am using this new abstraction... |
I'm guessing this is not your unclosed file -- updater currently leaves unclosed files on pretty much every "error" situation I think. Maybe check if same warning appears on develop branch? If it does then you don't need to handle it here. You can also get more details on the warning with |
Yes, those warnings are visible in the develop branch too. |
a9997e1
to
cf404d2
Compare
I left a few comments, otherwise I think it looks good |
Just wanted to say: thanks for doing all this tedious but necessary work! 🥇 |
5c6eb9f
to
1eb38a9
Compare
Together with the great help of @jku, we have been elaborating over those changes in the last couple of weeks and now I think this pr is ready for a review! I have rebased and cleared the commit history to make more sense for a newcomer. This pr hides the logs from the server because it's using |
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 for this great set of cleanups @MVrachev. I added a few minor comments and questions, but this is pretty much ready to merge. Excellent PR.
Signed-off-by: Martin Vrachev <[email protected]>
1eb38a9
to
e338cb9
Compare
I rebased my changes upon the latest changes in develop and changed the second commit |
e338cb9
to
9e384b9
Compare
I have decided that because I am working in the same code area I will use this pr to fix the issue #1119 too and that's what the last commit is all about. |
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.
LGTM.
Maybe you are aware but I'll mention for future commits: instead of
For reference read:
#1119
you can do Fixes #1119
(or use a few other keywords) to auto-close on merge -- obviously it's useful only if you know the issue should be closed when commit is merged.
Also, the format Fixes issue #N
does not seem to work -- this is why this PR cover letter is probably not going to trigger auto-closing either.
Thanks! |
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.
This is an excellent set of changes, thank you. I'd like to see a minor fixup of a docstring. After that, I think this is good to merge.
Logging the stdout and stderr from the test subprocesses into temporary files clean the console from unnecessary messages from the server-side such as "code 404, message File not found" or "GET" queries. I have decided to create TestServerProcess class that will handle the server subprocess creation and redirection to a temporary file object. That way that code can be reused in more than 10 files. Also, I have cleaned some parts of the unit test to make them more readable and efficient with the new abstraction. The unit tests are executed in sequential order and that's why we can reuse one temporary file object for multiple tests. Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
I don't see a need to leave a comment about what setupClass, tearDownClass, setup and tearDown functions do. There is documentation that describes that. Additionally, the links referenced in the comments are from Python 2 is deprecated. Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
Fixes an issue where rmtree tries to access and consequently remove a temp folder where the server has opened a file already. This results in error: "PermissionError: [WinError 32] The process cannot access the file because it is being used by another process" For reference read: theupdateframework#1119 Signed-off-by: Martin Vrachev <[email protected]>
b9ef099
to
e2ccfdb
Compare
Updated. |
Awesome work, thanks @MVrachev |
Fixes issues #: #1039, #1119
Description of the changes being introduced by the pull request:
This pr replaces pr #1041.
Logging the stdout and stderr from the test subprocesses into
temporary files clean the console from unnecessary messages from
the server-side such as "code 404, message File not found" or
"GET" queries.
The unit tests are executed in sequential order and that's why
we can reuse one temporary file object for multiple tests.
I have used this pr to remove some redundant functions, comments, and variables.
For this pr I received great help from @jku!
Thanks!
Please verify and check that the pull request fulfills the following
requirements: