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

Do not warn about no @Theme in views when in npm mode #6065

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

manolo
Copy link
Member

@manolo manolo commented Jul 14, 2019

This change is Reviewable

@project-bot project-bot bot added this to Review in progress in OLD Vaadin Flow ongoing work (Vaadin 10+) Jul 14, 2019
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo)


flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 652 at r1 (raw file):

        assert viewLocation != null;

        if (getSession().getConfiguration().isCompatibilityMode()) {

This fails in some tests with a NPE as session configuration is not set for the tests when using new MockUI().

java.lang.NullPointerException
	at com.vaadin.flow.component.internal.UIInternals.showRouteTarget(UIInternals.java:652)

@manolo manolo force-pushed the mcm/npm/remove-no-theme-logs branch from 2d2e528 to 5b9ef99 Compare July 15, 2019 06:18
Copy link
Member Author

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @caalador)


flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java, line 652 at r1 (raw file):

Previously, caalador wrote…

This fails in some tests with a NPE as session configuration is not set for the tests when using new MockUI().

java.lang.NullPointerException
	at com.vaadin.flow.component.internal.UIInternals.showRouteTarget(UIInternals.java:652)

Yep, Done

@manolo manolo force-pushed the mcm/npm/remove-no-theme-logs branch from 5b9ef99 to 5005c11 Compare July 15, 2019 06:51
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @manolo)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

        super(service);
        lock();
        if (service != null && service.getDeploymentConfiguration() != null) {

This has a problem that we set the deploymentConfiguration in other methods of the MockUI. They should now be changed to getDeploymentConfiguration().setXYZ() instead of using setDeploymentConfiguration

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, 1 of 1 files at r2.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @manolo)

Copy link
Member Author

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, caalador wrote…

This has a problem that we set the deploymentConfiguration in other methods of the MockUI. They should now be changed to getDeploymentConfiguration().setXYZ() instead of using setDeploymentConfiguration

right, I’m diving how to deal with this.

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @manolo)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

right, I’m diving how to deal with this.

MockUI could just always set a new MockDeploymentConfiguration() to the session if none is there and for the mode changes use the MockDeploymentConfiguration.setCompatibilityMode(boolean) instead of playing with a mock instance.

Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador and @manolo)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, caalador wrote…

MockUI could just always set a new MockDeploymentConfiguration() to the session if none is there and for the mode changes use the MockDeploymentConfiguration.setCompatibilityMode(boolean) instead of playing with a mock instance.

Also instead of a VaadinService mock it should set a new MockVaadinServletService()

@manolo manolo force-pushed the mcm/npm/remove-no-theme-logs branch from 5005c11 to 46ff0d2 Compare July 15, 2019 07:49
@manolo manolo force-pushed the mcm/npm/remove-no-theme-logs branch from 46ff0d2 to e19bfda Compare July 15, 2019 07:52
Copy link
Member Author

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, caalador wrote…

Also instead of a VaadinService mock it should set a new MockVaadinServletService()

MockUI already have static methods for creating them, but they cannot be used in failing tests because they need RouterTestUI instead, and other tests do not use the MockUI

Adjusted the failing tests so as they set correctly the configuration object before

Copy link
Member Author

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

MockUI already have static methods for creating them, but they cannot be used in failing tests because they need RouterTestUI instead, and other tests do not use the MockUI

Adjusted the failing tests so as they set correctly the configuration object before

BTW, I added a test for checking that theme is not computed in npm

Copy link
Member Author

@manolo manolo left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @caalador)


flow-server/src/test/java/com/vaadin/tests/util/AlwaysLockedVaadinSession.java, line 11 at r2 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

BTW, I added a test for checking that theme is not computed in npm

Done.

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Review in progress to Reviewer approved Jul 16, 2019
Copy link
Contributor

@caalador caalador left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, 1 of 1 LGTMs obtained

@manolo manolo merged commit 34ba7e8 into master Jul 16, 2019
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Reviewer approved to Done - pending release Jul 16, 2019
@manolo manolo deleted the mcm/npm/remove-no-theme-logs branch July 16, 2019 08:48
@mehdi-vaadin mehdi-vaadin added this to the 2.0.4 milestone Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

None yet

3 participants