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

[JAVA][Rest-assured] Added constants #697

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

viclovsky
Copy link
Contributor

@viclovsky viclovsky commented Jul 31, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Added constant "httpMethod", "summary" and fixed javadoc for "returnType", "dataType".

@wing328
Copy link
Member

wing328 commented Jul 31, 2018

@viclovsky thanks for the PR.

cc technical committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

public static final String REQ_URI = "{{path}}";
public static final String SUMMARY = "{{{summary}}}";
Copy link
Member

Choose a reason for hiding this comment

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

@viclovsky what's the use case for this constant storing the summary (which is included in the javadoc string above)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Constants are necessary to fill report.
Rest-assured is mostly client for testing. Tests often has integration with test report (such as Allure). Test report has detailed information about tests run results, curl arguments, command line etc.

Copy link
Member

Choose a reason for hiding this comment

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

@viclovsky thanks for the explanation 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It also intended to do the same with tags, but I haven't found easy way to do it.

@wing328 wing328 merged commit 82156b6 into OpenAPITools:master Aug 1, 2018
@jmini
Copy link
Member

jmini commented Aug 1, 2018

@viclovsky : the changes you made to the javadoc prevent the project to compile. When I run this:

mvn verify -f samples/client/petstore/java/rest-assured/

I get some errors like this:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:2.10.4:jar (attach-javadocs) on project petstore-rest-assured: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1 - /***/openapi-generator/samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/FakeApi.java:938: error: unknown tag: String
[ERROR]          * @param enumHeaderStringArray (List<String>) Header parameter enum test (string array) (optional)
[ERROR]                                              ^

See #43 for reference.

@wing328: why is the CI not catching this?

@wing328
Copy link
Member

wing328 commented Aug 1, 2018

@jmini no idea off the top of my head at the moment as the CI results are all green. I'll take a look at the log later to see if there are any clues

@wing328
Copy link
Member

wing328 commented Aug 1, 2018

UPDATE: I'm looking into the issue and trying to fix the CI (which should report red instead)

@wing328
Copy link
Member

wing328 commented Aug 1, 2018

Performed some more tests and CI (CircleCI) was able to report syntax errors with the Java rest-assured client. Still not entirely sure why the Javadoc error was missed (and I was able to repeat the Javadoc error locally)

@jeff9finger
Copy link
Contributor

I saw those changes to the javadoc, but did not think they should cause any issues. I guess the java doc generation is where things break. Is that right?

@wing328
Copy link
Member

wing328 commented Aug 1, 2018

Here is the error I got locally:

[ERROR] /Users/williamcheng/Code/openapi-generator/samples/client/petstore/java/rest-assured/src/main/java/org/openapitools/client/api/UserApi.java:271: error: unknown tag: User
[ERROR]          * @param user (List<User>) List of user object (required)
[ERROR]                             ^
[ERROR]
[ERROR] Command line was: /Library/Java/JavaVirtualMachines/jdk1.8.0_172.jdk/Contents/Home/jre/../bin/javadoc @options @packages
[ERROR]
[ERROR] Refer to the generated Javadoc files in '/Users/williamcheng/Code/openapi-generator/samples/client/petstore/java/rest-assured/target/apidocs' dir.
[ERROR]
[ERROR] -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

We suspect CircleCI failed to detect the error due to an old version of JDK8 used in the build environment.

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.

None yet

4 participants