Testing: Improve instructions and process #8844 #8845

tshradheya · 2018-05-23T09:32:30Z

Fixes #8844

Ongoing.

@LiHaoTan Need your input on the Intellij use default reporter

~~Would appreciate if someone can try out the following for me and check if it works on Eclipse~~

1. Change the language of eclipse IDE to German.
2. Run `TimeHelperTest` and ensure it fails
3. Make the necessary changes in `gradle.properties` as made in this PR
4. After rebuilding run the same test again and ensure it passes.

Outline of Solution

Made default reported being true so that its standardized for IntelliJ
Ensured tests are not locale-dependent
Corrected minor problem in documentation

LiHaoTan

Some queries

LiHaoTan · 2018-06-03T08:50:14Z

src/test/java/teammates/test/cases/datatransfer/StudentProfileAttributesTest.java

@@ -178,6 +179,10 @@ public void testToEntity() {
        StudentProfileAttributes testProfile = StudentProfileAttributes.valueOf(expectedEntity);
        StudentProfile actualEntity = testProfile.toEntity();

+        // Done to prevent instability of tests due to Instant.now()


Can you create a corresponding issue for this? Otherwise, this fix doesn't seem to be related.

Also, the way you explained this is a bit unclear. One may infer that there is some kind of problem with Instant.now().

Maybe something like: // toEntity does not use the original modified date so we need to ensure they are the same.

LiHaoTan · 2018-06-03T08:51:02Z

gradle.template.properties

@@ -8,3 +8,7 @@ org.gradle.daemon=false
 # e.g org.gradle.java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home
 # e.g org.gradle.java.home=C:/Program Files/Java/jdk1.8.0_144
 # org.gradle.java.home=
+
+# Use this property to have the recommended language setting for tests


This is not a recommendation since the user can't actually change it?

I think that will depend on if we keep it commented out or not be default?

I prefer it to be not commented out so that its the default

I mean the user doesn't really have any choice do they, the tests are dependent on the locale being minimally an English locale for now?

Ohh, sorry misunderstood your statement.

Yeah its not a recommendation but more of a requirement so that tests always pass irrespective of setting of the local computer.
Will change the comment accordingly

LiHaoTan · 2018-06-03T08:54:26Z

gradle.template.properties

@@ -8,3 +8,7 @@ org.gradle.daemon=false
 # e.g org.gradle.java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home
 # e.g org.gradle.java.home=C:/Program Files/Java/jdk1.8.0_144
 # org.gradle.java.home=
+
+# Use this property to have the recommended language setting for tests
+# It is used to prevent local language settings causing the tests to fail


It is a limitation that causes tests to fail if we don't have this set but this comment frame it as though it's the local language settings fault?

Maybe something like: # The test environment REQUIRES the following locale. Do not change!

Also setting only the language can create unsupported locale combinations such as en_CN though at least it appears Java have defaults that nicely aligned with what we need. So let's set -Duser.language=en -Duser.country=US?

Also is this sufficient to work in IDEs? Because this doesn't appear to work in IntelliJ?

Yes it works on IDE's. I set my locale to German and then on running the tests with these settings, they passed as expected

It doesn't appear to work for me in IntelliJ and it won't quite make sense because running tests in IntelliJ itself would not use the Gradle JVM.

How did you manage to make this work in IntelliJ? There must be something I'm missing here.

Can you confirm this?

If I am not mistaken, IntelliJ only supports English so this shouldn't be a problem itself.
On making my locale German and disabling the gradle JVM argument, the tests still pass when run through IntelliJ.

May I know what steps you took which didn't let this work on IntelliJ?

It is not possible to change the locale of the IDE but in the case of macOS if the user only has one language specified the JVM does not set user.language.format. So if my language is German, user.language will be set to de and the test will still fail in IntelliJ.

However, if I have more than one language in my operating system, user.language.format will be set to en in IntelliJ only since macOS High Sierra (http://www.openradar.me/34581827).

I didn't check how this works on Windows though.

You were right. I didn't think of this case.

On researching, I found that to get around this, we can set the user.country and user.language properties if IntelliJ. But currently, this can only be done by changing manually through idea.exe.vmoptions file or by passing through JVM arguments.

Better of this options is to set default JVM arguments, but I am not sure how to do it for any Run. Currently on CITests use -ea -Xss2m as default parameters. Do you know how can I make certain arguments as default when anything is run for this project?

For IntelliJ we can go Run... -> Edit Configurations... and set the defaults for TestNG.

We need to do this for Eclipse though as well.

Maybe it's easier if we just set the default locale in Java since the App Engine server will always be running in the locale en_US anyway.

For IntelliJ we can go Run... -> Edit Configurations... and set the defaults for TestNG.

Can we not do this in a similar manner of how its done for CI_Tests in runConfigurations in .idea folder?

just set the default locale in Java

Through Locale.setLocale() or by setting -Duser.country=US -Duser.language=en(not sure how to do this) as the default JVM arguments?

Can we not do this in a similar manner of how its done for CI_Tests in runConfigurations in .idea folder?

I'm afraid it seems that's the best we can do. At the moment we don't need to but perhaps if we ever need to we can modify workspace.xml. But it's probably easier to tell the user to set their defaults.

Share run configuration defaults: https://youtrack.jetbrains.com/issue/IDEA-65915

Through Locale.setLocale()

I meant this.

tshradheya · 2018-06-11T07:33:08Z

@LiHaoTan Ready for review

LiHaoTan · 2018-06-17T13:05:56Z

gradle.template.properties

@@ -8,3 +8,7 @@ org.gradle.daemon=false
 # e.g org.gradle.java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home
 # e.g org.gradle.java.home=C:/Program Files/Java/jdk1.8.0_144
 # org.gradle.java.home=
+
+# Use this property to have the required language setting for tests


I guess this line is kind of redundant?

LiHaoTan · 2018-07-07T11:36:07Z

@tshradheya this doesn't work for unit tests? Does any of the UI tests fail as well?

Looks like we have to set the locales in all the appropriate locations. Seems like this is more effort than anticipated that I overlooked.

I guess we don't really have a simple workaround so we should just do it the proper way. All the code that formats date is designed to support only en_US locale, so they should target that locale directly instead of depending on the default locale.

Then maybe we can consider changing the locale in Travis so at the very least we know that it works in more than one locale.

What do you think?

tshradheya · 2018-07-09T05:47:55Z

@tshradheya this doesn't work for unit tests? Does any of the UI tests fail as well?

I mistakenly updated locale in the wrong place. Updating locale in BaseTestCase, updates locale for all kinds of tests.
Now it works well for all tests. Now changing en_US to de_DE will fail the tests like TimeHelperTest hence we can verify that this method works

tshradheya · 2018-07-09T11:34:16Z

Looks like setting the locale default using Locale.setDefault() doesnt help for the UI test.

The rest work properly.
@LiHaoTan Any suggestion on how this problem can be tackled?

LiHaoTan · 2018-07-10T15:24:29Z

@tshradheya one problem at least comes from the server's locale generating dates that we are not expecting. I haven't quite thought through if there are any more cases where locales can affect the tests.

While we can work around that in a few ways such as setting locales just for tests only or changing the VM options to make the local server run with a specific locale, it seems like we have so many areas to consider.

Whereas the real problem is in that our code is really targeted at one locale only but we are writing the code as if it supports all locale.

That's why I was suggesting that code that formats dates (and numbers) such as DateTimeFormatter.ofPattern(processedPattern); to be changed to something like DateTimeFormatter.ofPattern(processedPattern, Locale.US);

This simplifies the following issue:

coupling. Tests have to be coupled to an execution environment, i.e. a specific locale

or from another perspective

magic. No matter what the locale is for some reason when we test formatting the format always ends up to be specific to en_US

tshradheya · 2018-07-11T08:01:53Z

Ready for review @LiHaoTan

Will revert .travis.yml changes soon. Now that build passes with new locale, we can be assured that this method works

LiHaoTan · 2018-07-14T03:16:10Z

src/main/java/teammates/common/util/TimeHelper.java

@@ -229,7 +230,7 @@ private static String formatLocalDateTime(LocalDateTime localDateTime, String pa
        if (localDateTime.getHour() == 12 && localDateTime.getMinute() == 0) {
            processedPattern = pattern.replace("a", "'NOON'");
        }
-        DateTimeFormatter formatter = DateTimeFormatter.ofPattern(processedPattern);
+        DateTimeFormatter formatter = DateTimeFormatter.ofPattern(processedPattern, Locale.US);


Shall we extract a TeammatesDateTimeFormatter using DateTimeFormatter.withLocale(Locale.US) for use?

Do you mean such that now the developer will need to write

DateTimeFormatter formatter = TeammatesDateTimeFormatter.ofPattern(somePattern);

And the ofPattern of TeammatesDateTimeFormatter will return a DateTimeFormatter with Locale set as Locale.US ?

LiHaoTan · 2018-07-14T05:47:17Z

src/main/java/teammates/common/util/StringHelper.java

@@ -181,7 +183,8 @@ public static String decrypt(String message) throws InvalidParametersException {
    }

    public static String toDecimalFormatString(double doubleVal) {


Let's generalize the method so we can reuse it everywhere (at the moment the method doesn't make sense anyway).

And let's use NumberFormat.getNumberInstance instead since that's the way Java designed it and also documented in the Java tutorials.

To obtain standard formats for a given locale, use the factory methods on NumberFormat such as getInstance or getCurrencyInstance. If you need only minor adjustments to a standard format, you can modify the format returned by a NumberFormat factory method.

https://docs.oracle.com/javase/8/docs/api/java/text/DecimalFormat.html#DecimalFormat-java.lang.String-java.text.DecimalFormatSymbols-

LiHaoTan · 2018-07-14T05:48:34Z

gradle.template.properties

@@ -8,3 +8,6 @@ org.gradle.daemon=false
 # e.g org.gradle.java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_144.jdk/Contents/Home
 # e.g org.gradle.java.home=C:/Program Files/Java/jdk1.8.0_144
 # org.gradle.java.home=
+
+# The test environment REQUIRES the following locale. Do not change!
+org.gradle.jvmargs=-Duser.language=en -Duser.country=US


You should remove this?

Yes. This workaround is no longer necessary :)

LiHaoTan · 2018-07-14T05:49:06Z

src/test/java/teammates/test/pageobjects/Browser.java

@@ -84,6 +84,7 @@ private WebDriver createWebDriver() {
            profile.setPreference("browser.helperApps.neverAsk.saveToDisk", "text/csv,application/vnd.ms-excel");
            profile.setPreference("browser.download.folderList", 2);
            profile.setPreference("browser.download.dir", System.getProperty("java.io.tmpdir"));
+            profile.setPreference("intl.accept_languages", "en-us, en");


Remove this?

LiHaoTan · 2018-07-14T05:49:11Z

src/test/java/teammates/test/pageobjects/Browser.java

@@ -93,6 +94,7 @@ private WebDriver createWebDriver() {

            ChromeOptions options = new ChromeOptions();
            options.addArguments("--allow-file-access-from-files");
+            options.addArguments("--lang=en-us");


Remove this?

LiHaoTan · 2018-07-14T05:49:36Z

src/test/java/teammates/test/cases/BaseTestCaseWithMinimalGaeEnvironment.java

        if (new URL(TestProperties.TEAMMATES_URL).getHost().contains(".appspot.com")) {
            SystemProperty.environment.set(SystemProperty.Environment.Value.Production);
        } else {
            SystemProperty.environment.set(SystemProperty.Environment.Value.Development);
        }
+
+        Locale.setDefault(new Locale("en", "US"));


Remove this?

LiHaoTan · 2018-07-14T05:49:49Z

src/test/java/teammates/test/cases/BaseTestCase.java

+     */
+    @BeforeSuite
+    public void setLocale() {
+        Locale.setDefault(new Locale("en", "US"));


Remove this?

Let's have this any way to ensure that all parts of test being run are using this Locale?

I think we should avoid this because the fact that tests are run with a different locale is hidden.

We could make it more explicit by renaming BaseTestCase but tests shouldn't be run using a different locale anyway unless it's to test some locale-specific behavior.

On a side note, it doesn't cover all cases so it might be worse to leave it there giving a false sense of working code. Or otherwise is it supposed to cover some cases but we didn't document it.

LiHaoTan · 2018-07-14T05:57:33Z

.travis.yml

@@ -28,6 +31,9 @@ before_install:
  - "/sbin/start-stop-daemon --start --quiet --pidfile /tmp/custom_xvfb_99.pid \
    --make-pidfile --background --exec /usr/bin/Xvfb -- :99 -ac -screen 0 1440x900x16"
  - nvm install node
+  - export LANG=de_DE.UTF-8


I'm thinking we should keep on using a different locale so it at least may catch issues when the code doesn't work across locales.

# Use a different locale from the default en_US to possibly catch errors in code not working across locales (although this does not ensure that the code is independent of all locales).

@tshradheya What do you think?

Maybe we can leave it as a comment in the travis.yml file so that it can be checked by developers in cases when huge PR's are being merged.

Because our original code was written only for Locale.US it seems unnecessary to put a different locale in our testing script just in hope to catch error if it fails for that one particular different locale

Our original code wasn't written for any locale, it was written without any considerations for any locale which is an issue.

But I guess it is also rather strange, even if the code supports more than one locale (as opposed to being locale independent) we won't include this. I suppose we can remove it altogether.

tshradheya · 2018-07-18T02:44:27Z

@LiHaoTan Ready for review

tshradheya mentioned this pull request May 24, 2018

IntelliJ cannot run failed tests #8740

Closed

tshradheya requested a review from LiHaoTan May 24, 2018 02:05

tshradheya added the s.ToReview The PR is waiting for review(s) label May 24, 2018

LiHaoTan requested changes Jun 3, 2018

View reviewed changes

LiHaoTan self-assigned this Jun 3, 2018

LiHaoTan added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jun 3, 2018

tshradheya requested a review from LiHaoTan June 4, 2018 08:41

tshradheya added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Jun 4, 2018

LiHaoTan reviewed Jun 17, 2018

View reviewed changes

tshradheya requested a review from LiHaoTan July 2, 2018 04:12

LiHaoTan requested changes Jul 14, 2018

View reviewed changes

tshradheya requested a review from LiHaoTan July 17, 2018 09:16

tshradheya closed this Jul 20, 2018

tshradheya force-pushed the improve-build-process branch from d981ebd to bc465d9 Compare July 20, 2018 09:33

This was referenced Jul 20, 2018

Testing: Improve instructions and process #8844 #8977

Closed

Testing: Improve instructions and process #8844 #8978

Closed

Testing: Improve instructions and process #8844 #8979

Closed

[#8844] Testing: Improve instructions and process #8988

Closed

		@@ -181,7 +183,8 @@ public static String decrypt(String message) throws InvalidParametersException {
		}

		public static String toDecimalFormatString(double doubleVal) {

Testing: Improve instructions and process #8844 #8845

Testing: Improve instructions and process #8844 #8845

Conversation

tshradheya commented May 23, 2018 • edited Loading

Fixes #8844

LiHaoTan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiHaoTan Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshradheya commented Jun 11, 2018

Choose a reason for hiding this comment

LiHaoTan commented Jul 7, 2018 • edited Loading

tshradheya commented Jul 9, 2018

tshradheya commented Jul 9, 2018

LiHaoTan commented Jul 10, 2018

tshradheya commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshradheya commented Jul 18, 2018

tshradheya commented May 23, 2018 •

edited

Loading

LiHaoTan Jun 4, 2018 •

edited

Loading

LiHaoTan Jun 24, 2018 •

edited

Loading

LiHaoTan commented Jul 7, 2018 •

edited

Loading