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

Make RouteData and EnterHandler serializable. #4145

Merged
merged 1 commit into from
May 22, 2018

Conversation

caalador
Copy link
Contributor

@caalador caalador commented May 21, 2018

Add IT test to test that a simple UI
can be serialized.

Fixes #4142
Fixes #4141


This change is Reviewable

Add IT test to test that a simple UI
can be serialized.
@caalador caalador self-assigned this May 21, 2018
@denis-anisimov
Copy link
Contributor

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/SerializeUIView.java, line 28 at r1 (raw file):

public class SerializeUIView extends AbstractDivView 

Is it necessary to have this is as a UI component with IT test ?
Is unit test not enough ?


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/TestingServiceInitListener.java, line 39 at r1 (raw file):

(UIInitListener

Shouildn't UIInitListener extend Serializable?


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/TestingServiceInitListener.java, line 41 at r1 (raw file):

BeforeEnterListener

same here


Comments from Reviewable

@caalador
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 3 unresolved discussions.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/SerializeUIView.java, line 28 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
public class SerializeUIView extends AbstractDivView 

Is it necessary to have this is as a UI component with IT test ?
Is unit test not enough ?

We have a junit test for serialization, but still we had the problem that in actual use we couldn't serialize a UI as the junit test didn't cover all the bases.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/TestingServiceInitListener.java, line 39 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
(UIInitListener

Shouildn't UIInitListener extend Serializable?

it does, but when serializing the lambda itself is not serializable


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/TestingServiceInitListener.java, line 41 at r1 (raw file):

Previously, denis-anisimov (Denis) wrote…
BeforeEnterListener

same here

it does, but when serializing the lambda itself is not serializable


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Review status: all files reviewed at latest revision, 2 unresolved discussions.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/SerializeUIView.java, line 28 at r1 (raw file):

Previously, caalador wrote…

We have a junit test for serialization, but still we had the problem that in actual use we couldn't serialize a UI as the junit test didn't cover all the bases.

Alright.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/ui/TestingServiceInitListener.java, line 39 at r1 (raw file):

Previously, caalador wrote…

it does, but when serializing the lambda itself is not serializable

Hm.....
If the API contract says that the method should accept the serializable object then why lambda is not serializable ? I would say it's a compiler mistake.
But OK, got it.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@denis-anisimov denis-anisimov merged commit decacfb into master May 22, 2018
@denis-anisimov denis-anisimov deleted the issues/serializable branch May 22, 2018 06:40
@pekam pekam added this to the 1.0.0.beta12 milestone May 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants