Adding default constructor to MockSerializationResult. #598

renanigt · 2014-06-15T13:15:01Z

Just adding a default constructor to make easier to use it in unit tests.

Now we need use some like: new MockSerializationResult(new JavassistProxifier(), cleanInstance(), new GsonBuilderWrapper(new MockInstanceImpl<>(jsonSerializers), new MockInstanceImpl<>(jsonDeserializers)));
With this default constructor, we just use some like: new MockSerializationResult();

What do you think ?

renanigt · 2014-06-15T13:22:13Z

I don't know why the tests didn't pass on Travis CI, when I run mvn test, they'll pass with success.

renanigt · 2014-06-15T13:34:37Z

When the Travis CI build, it doesn't know the class XStreamBuilderFactory, so the class exists as you know.
What I don't understand, why when I run mvn test on my machine, the tests pass with success.

renanigt · 2014-06-15T14:14:34Z

Sorry for some problem...
You can close this issue. 😞

But if you get fix this problem, I think it's better to work with unit tests.

garcia-jj · 2014-06-15T20:35:11Z

This error occurs to me too. Have you pushed all classes in your commit?

[ERROR] /home/otavio/eclipsew2/vraptor4/vraptor-core/src/main/java/br/com/caelum/vraptor/util/test/MockSerializationResult.java:[39,51] cannot find symbol
[ERROR] symbol:   class XStreamBuilderFactory
[ERROR] location: package br.com.caelum.vraptor.serialization.xstream
[ERROR] /home/otavio/eclipsew2/vraptor4/vraptor-core/src/main/java/br/com/caelum/vraptor/util/test/MockSerializationResult.java:[73,48] cannot find symbol
[ERROR] symbol:   variable XStreamBuilderFactory
[ERROR] location: class br.com.caelum.vraptor.util.test.MockSerializationResult

renanigt · 2014-06-15T20:46:58Z

Yes. I just created the default constructor.

It was based on this code:
https://github.com/caelum/vraptor4/blob/master/vraptor-core/src/test/java/br/com/caelum/vraptor/util/test/MockSerializationResultTest.java

garcia-jj · 2014-06-15T20:51:16Z

I have downloaded your code, and I got the same error that Travis reported. I think that you miss to commit anything.

Can you check this?

renanigt · 2014-06-15T20:52:44Z

I'll check this.
But I didn't create any classes, just the default constructor.

renanigt · 2014-06-15T21:18:12Z

I checked this and I have commited all.

I don't created anything, just the default constructor.

May you make a test for me, creating this default constructor and run mvn test ?

renanigt · 2014-06-15T21:20:41Z

Or you can close this issue and I try create with another branch.

renanigt · 2014-06-15T21:28:26Z

The problem is on the Symbol XStreamBuilderFactory.
This class exists on VRaptor, I don't know why this error.

renanigt · 2014-06-15T21:42:11Z

@garcia-jj May you run the test directly from Eclipse and after run mvn test ?
I got simulate the problem and after I ran with mvn test the problem was solved, I don't know why.

renanigt · 2014-06-15T21:52:54Z

Or just rebuild this branch for a test...
When occur the first time and I rebuild, the problem is solved.

I don't understand why.

garcia-jj · 2014-06-15T22:25:21Z

May be a missed import.
On Jun 15, 2014 6:28 PM, "Renan Montenegro" notifications@github.com
wrote:

The problem is on the Symbol XStreamBuilderFactory.
This class exists on VRaptor, I don't know why this error.

—
Reply to this email directly or view it on GitHub
#598 (comment).

renanigt · 2014-06-15T22:27:54Z

The import is there.
May run the teste from eclipse, just to make a test for me ?

renanigt · 2014-06-15T22:32:56Z

+import br.com.caelum.vraptor.serialization.xstream.XStreamBuilderFactory;

garcia-jj · 2014-06-15T22:40:44Z

Because XStreamBuilderFactory is located at test folder, you can't use in main folder, because when we create the vraptor jar, test folder is discarted.

renanigt · 2014-06-15T22:43:22Z

Oh Really... Sorry.

So, it's better closes this issue or fix it ?

garcia-jj · 2014-06-15T22:56:54Z

I don't know if it's a good idea to move a class that was designed to use in tests, to main folder. This default constructor is really necessary? Or we can live without?

renanigt · 2014-06-15T22:59:56Z

I think we can live without it, even if is easier using the dafault constructor.

So, we can after explain about on doc.

renanigt · 2014-06-16T00:07:29Z

But still I think it's so importante make the developer's life easier.

renanigt · 2014-06-16T00:08:42Z

Sorry for the english errors, I'm on cellphone. :-)

garcia-jj · 2014-06-16T00:31:15Z

Yeah, we love to make the developer's life easier. But in this case, test classes must located under test directory, and not in the main directory. And... are only 4 lines 👍

renanigt · 2014-06-16T00:40:20Z

I understand you.
I'll finish the controller's tests of vraptor-musicjungle and may be used as an example.

We can write something about MockSerializationResult on doc.

renanigt · 2014-06-16T03:34:05Z

I was reading the new book about VRaptor 4, is cited the use of MockSerializationResult as MockSerializationResult result = new MockSerializationResult();.

Also on VRaptor 3 we needed just the default constructor.

So it's very important think about this, because may cause confusion on developers and turn more difficult their life.

I think that move the class XStreamBuilderFactory to main folder may be a good idea and still make developer's life easier.

renanigt · 2014-06-16T03:48:53Z

I extracted the class to main folder, now the tests are passing.

So, if you think it's a good idea... 😃

renanigt · 2014-06-16T10:15:45Z

Before, the method cleanInstace was in XStreamBuilderImpl, so I moved to it and removed the XStreamBuilderFactory.

Turini · 2014-06-16T12:13:46Z

I don't think we should add this kind of class for vraptor-core main src.
If it is really so important, we can think about extract test utils for an plugin.

renanigt · 2014-06-16T12:18:55Z

I think It's important, but just a suggestion.

May be a good idea about a plugin.

Turini · 2014-06-16T12:22:24Z

What about wait some user ask for it? If (or when) that happens,
we can reopen the PR or extract for a plugin. What do you think?

renanigt · 2014-06-16T13:04:27Z

May be, I just think the book's example is a problem, but is a good idea.

Also we can write doc about MockSerializationResult.

lucascs · 2014-06-17T14:26:24Z

VRaptor has to be testable, and the package br.com.caelum.vraptor.util.test stands for helping the testability of controllers, interceptors, etc...

we cannot force the user to use a constructor that complicated to be able to test serialization. The no-args constructor is the best tool to help the user.

What do you think @Turini ?

lucascs · 2014-06-17T14:27:29Z

...or-core/src/test/java/br/com/caelum/vraptor/serialization/xstream/XStreamBuilderFactory.java

-/**
- * A Simple builder factory for XStreamBuilder tests
- */
-public class XStreamBuilderFactory {


Rollback this change. VRaptor is released, we cannot simply remove a class without bumping the major release version.

Turini · 2014-06-17T14:32:29Z

What do you think @Turini ?

for now I think we can add the defaut constructor...
maybe if test package grows we can extract to a test plugin.

But @renanigt, can you send a new PR only with this change?

renanigt · 2014-06-17T14:38:51Z

I made changes on test classes that used cleanInstance from XStreamBuilderFactory and now using from XStreamBuilderImpl, I rollback these changes too ?

renanigt · 2014-06-17T14:46:09Z

Oh, do you want a new PR ?
Sorry for this commit.

I'll send the new PR.

Turini · 2014-06-17T14:47:37Z

can you send a new PR only adding the constructor?

renanigt · 2014-06-17T14:48:16Z

Ok @Turini.
I'll do this. 😃

Turini · 2014-06-17T14:50:14Z

thanks! so closing here

lucascs · 2014-06-17T14:51:02Z

I think testing is core for any application/library, not meant to be a plugin.

renanigt · 2014-06-17T14:55:17Z

Make sense testing is core for any application.

garcia-jj · 2014-06-17T15:44:07Z

@lucascs You didn't invite me, but I will join discution nevertheless. I agree, but not fully. If you watch the code, this constructor is too specific.

As I wrote in mailing list, this pull request uses a JavassistProxifier. But if I need to use another proxifier implementation?

In this pull request, no converters are used. But if I my application have a converter?

So, this pull request may help users, but only in too specific cases. So I don't know if is too valuable to move to core.

renanigt · 2014-06-17T16:20:02Z

I have no knowledge as you ( @garcia-jj @Turini @lucascs ) and I'm learning a lot with you, but I'll left my opinion.

If you need use another constructor, in this case just need call the other constructor and not the default.
I think that in most cases the users will use just the default constructor, most of them don't know how to call the other because the dependecies.

garcia-jj · 2014-06-17T16:31:11Z

This is my point: this change is too specific to move to core.

renanigt · 2014-06-17T16:38:53Z

Seeing the class XStreamBuilderImpl, before, the method cleanInstance was there and after the method was extracted to XStreamBuilderFactory, so was moved from core to test folder.

I don't see as a problem.
With this, we make developer's life easier.
For me one of the great point of VRaptor is the testability, and for me we can't lost this.

lucascs · 2014-06-17T21:55:07Z

@garcia-jj I disagree, the change is not too specific. What if I have no converters, but I still want to know how my class is being serialized?

this needs to be easy.

rponte · 2014-06-17T22:06:38Z

I think it's a valuable change. It'll work very well for most of the cases.
If someone needs to register any converter or use a different proxy
implementation he just have to use another constructor and pass all
dependencies.

On Tuesday, June 17, 2014, Lucas Cavalcanti notifications@github.com
wrote:

@garcia-jj https://github.com/garcia-jj I disagree, the change is not
too specific. What if I have no converters, but I still want to know how my
class is being serialized?

this needs to be easy.

—
Reply to this email directly or view it on GitHub
#598 (comment).

Rafael Ponte
TriadWorks | Formação Java
http://cursos.triadworks.com.br

renanigt added 2 commits June 15, 2014 10:09

Adding default constructor to MockSerializationResult.

06bbf6d

Removing unnecessary attributes.

62cb08d

Just a removing a static import

fb52b9e

Just returning to inital stage.

33a3ddb

Adding the default constructor.

2c4fab2

Extracting the class XStreamBuilderFactory from tests to main folder

fb9fb55

Moving the method cleanInstance to XStreamBuilderImpl.

2890c4b

Adding default constructor on MockSerializationResultTest.

590109d

Turini closed this Jun 16, 2014

lucascs reopened this Jun 17, 2014

lucascs reviewed Jun 17, 2014
View reviewed changes

Rollback the removed class XStreamBuilderFactory.

3edcdf4

Turini closed this Jun 17, 2014

renanigt deleted the improveMockSerializationResult branch June 17, 2014 16:49

Adding default constructor to MockSerializationResult. #598

Adding default constructor to MockSerializationResult. #598

Conversation

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

garcia-jj commented Jun 15, 2014

renanigt commented Jun 15, 2014

garcia-jj commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

garcia-jj commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 15, 2014

garcia-jj commented Jun 15, 2014

renanigt commented Jun 15, 2014

garcia-jj commented Jun 15, 2014

renanigt commented Jun 15, 2014

renanigt commented Jun 16, 2014

renanigt commented Jun 16, 2014

garcia-jj commented Jun 16, 2014

renanigt commented Jun 16, 2014

renanigt commented Jun 16, 2014

renanigt commented Jun 16, 2014

renanigt commented Jun 16, 2014

Turini commented Jun 16, 2014

renanigt commented Jun 16, 2014

Turini commented Jun 16, 2014

renanigt commented Jun 16, 2014

lucascs commented Jun 17, 2014

lucascs Jun 17, 2014

Choose a reason for hiding this comment

Turini commented Jun 17, 2014

renanigt commented Jun 17, 2014

renanigt commented Jun 17, 2014

Turini commented Jun 17, 2014

renanigt commented Jun 17, 2014

Turini commented Jun 17, 2014

lucascs commented Jun 17, 2014

renanigt commented Jun 17, 2014

garcia-jj commented Jun 17, 2014

renanigt commented Jun 17, 2014

garcia-jj commented Jun 17, 2014

renanigt commented Jun 17, 2014

lucascs commented Jun 17, 2014

rponte commented Jun 17, 2014