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

Use the new router by default #2929

Merged
merged 19 commits into from
Nov 16, 2017
Merged

Use the new router by default #2929

merged 19 commits into from
Nov 16, 2017

Conversation

caalador
Copy link
Contributor

@caalador caalador commented Nov 15, 2017

This change is Reviewable

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2017

CLA assistant check
All committers have signed the CLA.

@denis-anisimov
Copy link
Contributor

Reviewed 65 of 67 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


flow-server/src/test/java/com/vaadin/server/MockVaadinServletService.java, line 53 at r2 (raw file):

e.printStackTrace();

Are you sure that it shouldn't throw a runtime exception ?


flow-test-util/src/main/java/com/vaadin/flow/testutil/AbstractParallelTestBenchTest.java, line 70 at r2 (raw file):

public void setup() throws Exception {

Quoted 5 lines of code… > if(USE_HUB) { > setDesiredCapabilities(Browser.CHROME.getDesiredCapabilities()); > } > super.setup(); > }

What's the need for those changes ?


flow-test-util/src/main/java/com/vaadin/flow/testutil/AbstractParallelTestBenchTest.java, line 116 at r2 (raw file):

@OverRide

Quoted 5 lines of code… > protected Browser getRunLocallyBrowser() { > if (USE_HUB) { > return null; > } > return Browser.CHROME;

What's the need for those changes ?


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/servlet/ApplicationRunnerServlet.java, line 286 at r2 (raw file):

e.printStackTrace();

Throw a runtime exception ?


flow-tests/test-subcontext/src/main/java/com/vaadin/flow/contexttest/ui/DependencyUI.java, line 59 at r2 (raw file):

// Don't try to use router just display the

display what ?


flow-tests/test-subcontext/src/main/java/com/vaadin/flow/contexttest/ui/DependencyUI.java, line 72 at r2 (raw file):

e.printStackTrace();

Throw a runtime exception ?


Comments from Reviewable

Router is configured only if there are routes.
@caalador
Copy link
Contributor Author

Review status: 43 of 75 files reviewed at latest revision, 6 unresolved discussions.


flow-test-util/src/main/java/com/vaadin/flow/testutil/AbstractParallelTestBenchTest.java, line 70 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

public void setup() throws Exception {
if(USE_HUB) {
setDesiredCapabilities(Browser.CHROME.getDesiredCapabilities());
}
super.setup();
}

What's the need for those changes ?

For some reason with merge from master TB started to default to FireFox and not read the BrowserConfiguration.


flow-test-util/src/main/java/com/vaadin/flow/testutil/AbstractParallelTestBenchTest.java, line 116 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

@OverRide
protected Browser getRunLocallyBrowser() {
if (USE_HUB) {
return null;
}
return Browser.CHROME;

What's the need for those changes ?

Local browser was not gotten from the same place as before and tests were trying to run only on the test hub target localhost:4444/...


flow-tests/test-subcontext/src/main/java/com/vaadin/flow/contexttest/ui/DependencyUI.java, line 59 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

// Don't try to use router just display the

display what ?

it was supposed to end with UI. Removed due to small change to Router.


flow-tests/test-subcontext/src/main/java/com/vaadin/flow/contexttest/ui/DependencyUI.java, line 72 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

e.printStackTrace();

Throw a runtime exception ?

This is removed. Updated router.


flow-tests/test-root-context/src/main/java/com/vaadin/flow/uitest/servlet/ApplicationRunnerServlet.java, line 286 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

e.printStackTrace();

Throw a runtime exception ?

This is removed. Updated router and moved pure UI tests to own module.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 56 of 56 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

This is fine as it will also be removed when components
are published from their own repositories.
@caalador
Copy link
Contributor Author

Review status: 73 of 76 files reviewed at latest revision, 2 unresolved discussions.


flow-server/src/test/java/com/vaadin/server/MockVaadinServletService.java, line 53 at r2 (raw file):

Previously, denis-anisimov (Denis) wrote…

e.printStackTrace();

Are you sure that it shouldn't throw a runtime exception ?

Done.


Comments from Reviewable

@denis-anisimov
Copy link
Contributor

Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@caalador caalador merged commit 789654d into master Nov 16, 2017
@caalador caalador deleted the issues/new_router_by_default branch November 16, 2017 11:41
@caalador caalador added this to the 1.0.0.alpha11 milestone Nov 23, 2017
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