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

HWKMETRICS 185 - JAX-RS 1.1 implementation #308

Merged
merged 78 commits into from Aug 14, 2015
Merged

HWKMETRICS 185 - JAX-RS 1.1 implementation #308

merged 78 commits into from Aug 14, 2015

Conversation

stefannegrea
Copy link
Contributor

No description provided.

Stefan Negrea added 30 commits August 13, 2015 11:37
…n module and enable it in the new JAX-RS 1.1 implementation.
…plementation is not working properly with the current annotations.
@jsanda
Copy link
Contributor

jsanda commented Aug 14, 2015

I have two concerns. First, are there any problems with sharing the REST model classes since we have two different sets of annotations? For the JAX-RS 2 impl, do we need Jackson 1.9 at runtime? And for the JAX-RS 1.1 impl do we need Jackson 2 at runtime?

Secondly, we need to fix the integration test situation. Right now if I want to test a change against the JAX-RS 1.1 impl that involves changing or adding a new REST API test, I would,

  • Update the rest-tests module
  • Build rest-tests with ``mvn install`
  • cd tests/rest-integration-tests-jaxrs-1.1
  • mvn verify (from tests/rest-integration-tests-jaxrs-1.1 directory)

We want the REST API tests to run against both implementations, and this multi-step process will inevitably cause problems such as forgetting to run the tests against the JAX-RS 1.1 impl. The second issue is that you cannot even run the new or updated tests against the 1.1 impl until we have it passing for the 2.0 impl. That's no good either. Ideally, running ``mvn install` in the rest-tests module should by default execute tests against both impls. And we should provide a switch (via system property/profile) to run against one or the other.

@stefannegrea
Copy link
Contributor Author

There are no issues with having two sets of annotations on the common model classes because the annotations are in different packages. The automated tests currently deploy the app on both servers (WildFly 9 and EAP 6.4 Beta1) and there are no classloader issues detected. It would have been a huge problem if there was an overlap between the two implementations (same class, same package name, but different features and code).

However, there are a few minor annoyances. First, the common package is going to have extra dependencies for each JAX-RS implementation. Second, there are double annotations on each class. And third, the annotations classes require full path (package name + class name) to avoid the actual class name clash.

The alternative is to have even more code duplication between two JAX-RS implementations. This way, with just a few minor annoyances, the duplicated code is reduced by a good chunk.

@stefannegrea
Copy link
Contributor Author

Now about the integration tests. Due to heavy refactoring, I was not able to move the rest-tests under the tests module. @jsanda, what you are proposing is on my list of things to do post merge.

The plan is to relocate all the tests under the tests module as sub-modules. Add a pom at that level and all what you have to do is to cd into that folder and do mvn install a single time and run all the tests at once.

@jsanda
Copy link
Contributor

jsanda commented Aug 14, 2015

However, there are a few minor annoyances. First, the common package is going to have extra dependencies for each JAX-RS implementation. Second, there are double annotations on each class. And third, the annotations classes require full path (package name + class name) to avoid the actual class name clash.

Do we need both versions of Jackson at runtime? If so, does this raise any productization concerns?

The double annotations and using the FQCNs is minor especially if it means we can avoid duplicating the REST model classes.

@stefannegrea
Copy link
Contributor Author

Both versions are packaged in the api-commons jar when the module is built. From a delivery perspective, luckily both are productized as part of either EAP 6.4 or other projects.

@jsanda
Copy link
Contributor

jsanda commented Aug 14, 2015

The plan is to relocate all the tests under the tests module as sub-modules. Add a pom at that level and all what you have to do is to cd into that folder and do mvn install a single time and run all the tests at once.

Will this allow us to,

  • Write one set of tests for both impls
  • Execute tests for both impls in one build
  • Add tests and execute them for the 1.1 impl without first having to build them again the 2.0 impl

@stefannegrea
Copy link
Contributor Author

@jsanda, the first two points are covered.

And the third point can be half done easily, although I am not sure it is a good idea. That is, you can write and execute tests just for one implementation. As far as not building the 2.0 test suite, that is not easy without having a common module. But, I do not think we need this since we want all the tests to pass on both implementations all the time. While there is a practical argument for saving a little time, it is safer to just assume that tests will always run on both implementations and that building the 2.0 suite first is required.

@jsanda
Copy link
Contributor

jsanda commented Aug 14, 2015

I agree that we want the tests to run against both impls, but there will be times where we want to focus on one or the other. For instance, If I make a change in the 1.1 impl, it is understandable that I would want to run the tests against the 1.1 impl first to make sure there are no regressions. I know we want to get these changes in master yesterday so I am fine with continuing to discuss post merge. Would you mind though capturing the relevant info in a ticket?

@stefannegrea
Copy link
Contributor Author

One detail for consideration, you would only need to build once the suite for 2.0 implementation. Once it is built it can be reused to test the 1.1 implementation as many times as needed, without the need to rebuild 2.0 implementation. And the only time to rebuild the 2.0 implementation is when you tests are added.

My goal is to relocate the rest-tests for 2.0 immediately after the merge. This was not possible now because I needed to constantly rebase on master to keep the changes flowing to this branch.

jsanda pushed a commit that referenced this pull request Aug 14, 2015
HWKMETRICS 185 - JAX-RS 1.1 implementation
@jsanda jsanda merged commit 24d662e into master Aug 14, 2015
@stefannegrea
Copy link
Contributor Author

Create the following JIRA to track the request from @jsanda comments above: https://issues.jboss.org/browse/HWKMETRICS-216

@stefannegrea stefannegrea deleted the HWKMETRICS-185 branch September 29, 2015 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants