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

test: Linux local build fixes #9942

Merged
merged 6 commits into from
Feb 2, 2021
Merged

test: Linux local build fixes #9942

merged 6 commits into from
Feb 2, 2021

Conversation

miguelatvaadin
Copy link
Contributor

These commits fix some issues which arise while locally building the project on a linux desktop

BinderTest#withConverter_writeBackValue was failing depending on the locale. Now it's locale aware and does not fail when run on any (e.g. spanish) locale
…uriWithDirectoryChange_statusForbidden

The UrlValidationIT#devModeUriValidation_uriWithDirectoryChange_statusForbidden test was failing due to a race condition. Now it checks the response body to verify that the frontend is compiled, and it performs as expected
@CLAassistant
Copy link

CLAassistant commented Feb 2, 2021

CLA assistant check
All committers have signed the CLA.

@mshabarov
Copy link
Contributor

Thanks for fixing the timing issue, @miguelatvaadin ! I was not taking it into consideration when made this test.

@mshabarov mshabarov changed the title Linux local build fixes test: Linux local build fixes Feb 2, 2021
mshabarov
mshabarov previously approved these changes Feb 2, 2021
}

private String getResponseBody(HttpURLConnection connection) throws IOException {
String body = String.join("\n", IOUtils.readLines(connection.getInputStream(), StandardCharsets.UTF_8));
Copy link
Contributor

@taefi taefi Feb 2, 2021

Choose a reason for hiding this comment

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

I think we had this policy of not using Apahce Commons and etc. because of versioning conflicts and the challenges it creates for the users and should be avoided. However, this comment is just a reminder, since this is a test code. No action needed IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I saw it in the wiki. Anyway, IOUtils was already used in several points in the project, even in flow-server, so I understand it's not a problem.

Comment on lines 26 to 30
import com.vaadin.flow.testutil.ChromeBrowserTest;

import org.apache.commons.io.IOUtils;
import org.junit.Assert;
import org.junit.Test;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe com.vaadin.* imports should appear after all other imports and before elemental.* and import static statements. Although the code was like that even before these changes, but it's a chance to fix them here ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I will change this. Thanks, Soroosh ;)

@mshabarov
Copy link
Contributor

Needs a merge with latest master to not complain about chrome version.

@miguelatvaadin
Copy link
Contributor Author

Needs a merge with latest master to not complain about chrome version.

Ok. I go for it. Thanks!

@taefi taefi merged commit 9abfea1 into master Feb 2, 2021
OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Feb 2, 2021
@taefi taefi deleted the linux-local-build-fixes branch February 2, 2021 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
OLD Vaadin Flow ongoing work (Vaadin ...
  
Done - pending release
Development

Successfully merging this pull request may close these issues.

None yet

5 participants