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

Fix get console output not working with Resteasy connector (#795) #949

Merged
merged 2 commits into from
Mar 2, 2017

Conversation

lorenzo-biava
Copy link
Contributor

This PR replaces the JSON String encoding of the compute.getConsoleOutput API request with a standard EntityModel so that it fixes issue #795.

With this entity modification, it also changes the default console length, which is now the full console instead of 50 lines.

Some unit tests has been added for this API.

@lorenzo-biava lorenzo-biava changed the title [WIP] Fix get console output not working with Resteasy connector #795 Fix get console output not working with Resteasy connector (#795) Feb 28, 2017
@lorenzo-biava
Copy link
Contributor Author

@auhlig It's ready for me. Let me know if there's anything that needs improvement...

@auhlig
Copy link
Member

auhlig commented Mar 2, 2017

Thanks for all the work @lorenzo-biava.
Could you explain the need for the takeRequest() method in AbstractTest?
It works without and if necessary, you can always use server.takeRequest() in the respective test classes that extend AbstractTest.
Besides this looks good.

@lorenzo-biava
Copy link
Contributor Author

lorenzo-biava commented Mar 2, 2017

Sure. I wanted to make sure that the console output lenght logic was correct and so I needed to check the request for the length parameter presence/absence.
So I added a common takeRequest() method for a couple of reasons, even though it is not absolutely necessary.

The first is that I thought that an abstraction on the mock server like respondWith() would be more consistent (and perhaps the takeRequest() method might have additional logic in the future).

The second, more practical one, is that in the createServer test there's already a variable named server, so it would clash with the mock server field.

I can change the variable in the createServer() method or use this.server.takeRequest() if you prefer, but I felt it would be less confusing having a dedicated common method to access common mock server functionalities indirectly.

@auhlig
Copy link
Member

auhlig commented Mar 2, 2017

Makes sense. Thanks @lorenzo-biava. Merging now.

@auhlig auhlig merged commit 870fb10 into ContainX:master Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants