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

Fixes #9201: Add basic infrastructure to test REST API #1227

Conversation

fanf
Copy link
Member

@fanf fanf commented Oct 2, 2016

@fanf fanf force-pushed the arch_9201/add_basic_infrastructure_to_test_rest_api branch from bc845df to 198c550 Compare November 4, 2016 17:40
@fanf
Copy link
Member Author

fanf commented Nov 4, 2016

PR rebased

Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

This iss great - i'm just not sure avout the "plop" - shouldn't it fail ?

method: GET
url: /api/techniqueLibrary/reload
response:
type: plop
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you want this type ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I wanted to test parsing error.
For now, the behaviour is to log:

[2016-11-15 11:33:32] ERROR com.normation.rudder.web.rest.TestRestFromFileDef - I wasn't able to run the 2th tests in file api/api_v1.yml <- The YAML document is empty
[2016-11-15 11:33:33] ERROR com.normation.rudder.web.rest.TestRestFromFileDef - I wasn't able to run the 1th tests in file api/api_v2.yml <- Unrecognized response type: 'plop'. Use 'json' or 'text'

And the test is not run. Would you find better to also fail the test? Effectively, it could be easier to correct all problems when we will change the format.

Copy link
Member

Choose a reason for hiding this comment

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

yes, i'd rather enjoy a fail

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it's better to have failing tests, but now I can't commit them because they will fails the test... not sur how to handle that.



def GET(path: String) = {
val mockReq = new MockHttpServletRequest("http://localhost:8080")
Copy link
Member

Choose a reason for hiding this comment

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

probably nitpicking, but if jetty listen on another port, it will fail
we should probably use the defined port for jetty if available.
But that's probably for a future PR

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK, the server/port is only used to build the fake Request/Response, not to actually send anything on that URL. It could be anything, I believe.

Copy link
Member

Choose a reason for hiding this comment

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

ha ok !

/*
* I *love* java. The snakeyaml parser returns a list of Objects. The may be null if empty, or
* others things. We don't know what other things. So I believe we just hope and cast to
* a Map[String, Object]. Which not much better. Expects other cast along the line.
Copy link
Member

Choose a reason for hiding this comment

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

i share your pain

@fanf
Copy link
Member Author

fanf commented Nov 15, 2016

PR rebased

@fanf fanf force-pushed the arch_9201/add_basic_infrastructure_to_test_rest_api branch from 198c550 to 4526264 Compare November 15, 2016 11:09
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit 4526264 into Normation:branches/rudder/3.1 Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants