Skip to content

Conversation

rustyscottweber
Copy link

This should put to rest Issue 72. This expands the capabilities of pywinrm to be able to send input to an already running process.

@rustyscottweber
Copy link
Author

My suggestion is that we create a release for this once checked in.

@coveralls
Copy link

coveralls commented Aug 10, 2019

Coverage Status

Coverage increased (+0.3%) to 76.797% when pulling ccc8cff on rustyscottweber:stdin_development into fe59460 on diyan:master.

@rustyscottweber
Copy link
Author

@diyan , I'm having some difficulties understanding your unit tests, it would appear that you have mocked the function for returning a message, but the response is predetermined off of what the message being sent was. I'm not sure how the xml in these sections should look. Can you lend a hand and tell me how I can figure out how I can patch the tests to get this function checked in or finish up the tests yourself?

@badcure
Copy link
Collaborator

badcure commented Aug 11, 2019

@rustyscottweber The XML location currently is at https://github.com/diyan/pywinrm/blob/master/winrm/tests/conftest.py . These are just mocked responses, and what I believe how py.test sets up each test.

That said, I am hoping to redo how the unit tests are done. The initial PR is #270 . Feel free to look over that as well if you have any comments, I think once that is merged...I hope it will be a little easier to understand/write tests.

@rustyscottweber
Copy link
Author

rustyscottweber commented Aug 11, 2019 via email

Copy link
Collaborator

@badcure badcure left a comment

Choose a reason for hiding this comment

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

And yes, please include unit tests. At the very least, it should ensure the code works.

Looking at conftest.py, you will have the provide the proper request xml(i.e. the one you are creating) and return a response. I think you tie it all in over here: https://github.com/diyan/pywinrm/blob/master/winrm/tests/conftest.py#L332-L352

Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Would be great if you could also add the tests for some of the encoding examples I had in my comments.

@rustyscottweber
Copy link
Author

Are there any more outstanding issues? Or can this be merged?

@rustyscottweber
Copy link
Author

I believe that there are no longer any outstanding issues. Is there anything else that needs to be reviewed before merging?

Copy link
Collaborator

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

The issues I had have been fixed, up to @badcure to do the final merge once he's ready.

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.

4 participants