Skip to content
This repository was archived by the owner on Sep 16, 2021. It is now read-only.

Fixes #262 - Integration testing more stable in maven. #283

Merged
merged 3 commits into from
Jun 9, 2016

Conversation

joakime
Copy link

@joakime joakime commented Jun 9, 2016

  • Forces maven-failsafe-plugin in parent (with version)
  • Moves jetty9-compact-base tests back into src/test/java
  • Renames integration tests from *Test.java to *IT.java to
    better conform to new recommendations from maven-failsafe-plugin
    (convention over configuration!)

…ven.

+ Forces maven-failsafe-plugin in parent (with version)
+ Moves jetty9-compact-base tests back into src/test/java
+ Renames integration tests from *Test.java to *IT.java to
  better conform to new recommendations from maven-failsafe-plugin
  (convention over configuration!)
@joakime
Copy link
Author

joakime commented Jun 9, 2016

This PR isn't sane (yet), a followup commit (to correct checkstyle warnings) is due for this PR.

@joakime
Copy link
Author

joakime commented Jun 9, 2016

Google Style Guide is incompatible with maven-failsafe-plugin.

/home/joakim/code/jetty/appengine-java-vm-runtime/jetty9-compat-base/src/test/java/com/google/apphosting/vmruntime/jetty9/VmRuntimeJettyAuthIT.java:30: error: Abbreviation in name must contain no more than '1' capital letters.

We'll need an exception.

@joakime joakime self-assigned this Jun 9, 2016
@joakime
Copy link
Author

joakime commented Jun 9, 2016

@aozarov do you want me to alter the Checkstyle xml?

@@ -68,12 +62,18 @@
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpSession;

import static org.easymock.EasyMock.createMock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this was changed. static imports should be at the top according to the Google style guide

@aozarov
Copy link
Contributor

aozarov commented Jun 9, 2016

@aozarov do you want me to alter the Checkstyle xml?

Please do. it should be fine to have an IT suffix for integration tests and exempt it from checkstyle.

@joakime
Copy link
Author

joakime commented Jun 9, 2016

@aozarov why didn't the google-java-format-1.0-all-deps.jar fix these import issues?

@joakime
Copy link
Author

joakime commented Jun 9, 2016

Ah, import management is currently broken in the google-java-format-1.0-all-deps.jar

Also, looks like there's a googleformat-maven-plugin in existence (maintained by @talios)

+ Fixed import order issues
+ Added checkstyle/checkstyle-suppressions.xml
+ Updated root pom.xml to use new suppressions techniques in a way
  that's friendly to a maven reactor build
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">

<suppressions>
<suppress files="[a-zA-Z0-9]*IT.java" checks="AbbreviationAsWordInName"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, should it be prefixed with [/\\]test[/\\]src[/\\]java[/\\] (also for the other IT, Test checks)?

Copy link
Author

Choose a reason for hiding this comment

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

I would vote for the simpler form, as maven is free to copy/move files around into the /target/ (aka ${project.build.directory}) directory, which checkstyle also looks inside of.

Copy link
Contributor

@aozarov aozarov Jun 9, 2016

Choose a reason for hiding this comment

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

I can live with it though I wish we could avoid being lenient where we shouldn't.

@aozarov aozarov merged commit b9e3865 into GoogleCloudPlatform:async-support Jun 9, 2016
@joakime joakime mentioned this pull request Jun 10, 2016
@joakime joakime deleted the bugs/262 branch June 10, 2016 21:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants