L10n tests and better error handling #421

vwoodzell · 2015-11-11T01:59:58Z

Added some unit tests for l10n. Also changed addL10nSubstitutionInner() to address some problems like the one in bug 6386. Added a test that validates the l10n files, and fixed problems revealed by that test.

Bombe · 2015-11-11T06:03:12Z

Looks good.

bertm · 2015-11-11T09:22:55Z

src/freenet/l10n/BaseL10n.java

+        try {
+            addL10nSubstitutionInner(tempNode, value, patterns, values);
+        } catch (L10nParseException e) {
+            Logger.error(this, "Error in l10n value \""+value+"\" for "+key+": "+e.getMessage());


There's Logger.error(Object o, String s, Throwable e) for logging exceptions (it also logs the stack trace, which might be useful).

bertm · 2015-11-11T09:51:23Z

This is not a blocker, but something I'd like to add anyhow.

I find the naming of the methods confusing. You currently have:
addL10nSubstitution, which wraps around
getL10nSubstitution, which wraps around
tryGetL10nSubstitution, which wraps around
addL10nSubstitutionInner

Only the former two are specific to l10n (they pass / lookup keys), the latter two are generally applicable pattern substitution methods. Given the length of this chain, it's a bit odd to call addL10nSubstitutionInner the inner method of addL10nSubstitution.

As a suggestion, the following might be more clear:

addL10nSubstitution (public method, probably shouldn't be renamed),
getSubstitutionByKey,
tryGetSubstitutionByValue,
addSubstitutionByValue

bertm · 2015-11-11T09:57:48Z

Looks pretty sane in general, I only had some minor comments. Special thanks for creating the unit tests!

Add some unit tests for BaseL10n.getString() and addL10nSubstitution().

Throw exceptions in BaseL10n.addL10nSubstitutionInner() when problems are encountered. Add wrapping tryAddL10nSubstitution() method which only applies changes to the node if no exception is thrown.

Always clone node before adding to parent in BaseL10n.addL10nSubstitutionInner(), to avoid errors and for consistency.

If there's an error in the l10n string in BaseL10n.addL10nSubstitutionInner(), use the default string instead.

Add test that tries to parse all l10n strings in order to find invalid ones.

If we encounter errors in both the current language's value and the fallback value for a key, return the key itself from addL10nSubstitution().

Pass l10n parse exception into Logger.error() instead of extracting the message ourselves.

Implement an iterator over the current language string, fallback language string, and literal key. Use in place of logic in getString(), getDefaultString(), and getL10nSubstitution().

vwoodzell · 2015-11-13T04:23:47Z

Commits have "changed" due only to merging the first two. Other changes are in new commits.

Thynix · 2015-12-07T01:37:09Z

These tests do not pass on my machine. Have I misconfigured something perhaps? I ran ant clean; ant from the root directory of the repository.

[junit] Running freenet.l10n.BaseL10nTest
[junit] Testsuite: freenet.l10n.BaseL10nTest
[junit] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.095 sec
[junit] Tests run: 2, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.095 sec
[junit] ------------- Standard Error -----------------
[junit] Could not get resource : ../test/freenet/l10n/freenet.l10n.en.properties
[junit] Could not get resource : ../test/freenet/l10n/freenet.l10n.de.properties
[junit] Could not get resource : ../test/freenet/l10n/freenet.l10n.en.properties
[junit] The default translation for test.substitution hasn't been found!
[junit] java.lang.Exception
[junit]     at freenet.l10n.BaseL10n.getFallbackString(BaseL10n.java:544)
[junit]     at freenet.l10n.BaseL10n.access$000(BaseL10n.java:37)
[junit]     at freenet.l10n.BaseL10n$L10nStringIterator.next(BaseL10n.java:165)
[junit]     at freenet.l10n.BaseL10n$L10nStringIterator.next(BaseL10n.java:139)
[junit]     at freenet.l10n.BaseL10n.getHTMLWithSubstitutions(BaseL10n.java:702)
[junit]     at freenet.l10n.BaseL10n.addL10nSubstitution(BaseL10n.java:683)
[junit]     at freenet.l10n.BaseL10nTest.testAddL10nSubstitutionMissingFallback(BaseL10nTest.java:156)
[junit]     at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
[junit]     at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
[junit]     at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
[junit]     at java.lang.reflect.Method.invoke(Method.java:606)
[junit]     at junit.framework.TestCase.runTest(TestCase.java:176)
[junit]     at junit.framework.TestCase.runBare(TestCase.java:141)
[junit]     at junit.framework.TestResult$1.protect(TestResult.java:122)
[junit]     at junit.framework.TestResult.runProtected(TestResult.java:142)
[junit]     at junit.framework.TestResult.run(TestResult.java:125)
[junit]     at junit.framework.TestCase.run(TestCase.java:129)
[junit]     at junit.framework.TestSuite.runTest(TestSuite.java:255)
[junit]     at junit.framework.TestSuite.run(TestSuite.java:250)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.run(JUnitTestRunner.java:535)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.launch(JUnitTestRunner.java:1182)
[junit]     at org.apache.tools.ant.taskdefs.optional.junit.JUnitTestRunner.main(JUnitTestRunner.java:1033)
[junit] ------------- ---------------- ---------------
[junit] 
[junit] Testcase: testAddL10nSubstitutionMissingBrace took 0.026 sec
[junit] Testcase: testAddL10nSubstitutionMissingFallback took 0.006 sec
[junit]     FAILED
[junit] expected:<[Text with <b>loud</b> string]> but was:<[test.substitution]>
[junit] junit.framework.ComparisonFailure: expected:<[Text with <b>loud</b> string]> but was:<[test.substitution]>
[junit]     at freenet.l10n.BaseL10nTest.testAddL10nSubstitutionMissingFallback(BaseL10nTest.java:159)
[junit]

Change buildfile to copy l10n test properties files into the unit test build dir.

vwoodzell · 2015-12-07T16:24:26Z

Ah, my mistake. I was running without "ant clean", and eclipse had previously copied the test properties files into the build dir. I've added a task to the buildfile to copy them.

Thynix · 2015-12-29T02:55:06Z

Turns out these don't pass in Java 6:

[junit] Testcase: testGetStringOverridden took 0.005 sec
[junit]     FAILED
[junit] expected:<[O]verridden> but was:<[Not o]verridden>
[junit] junit.framework.ComparisonFailure: expected:<[O]verridden> but was:<[Not o]verridden>
[junit]     at freenet.l10n.BaseL10nTest.testGetStringOverridden(BaseL10nTest.java:171)
[junit]

I'll look into it. Any ideas why?

Thynix · 2015-12-31T16:56:36Z

When it breaks under Java 6 it's looking for the override file here: "/home/steve/freenet/fred/run/file:/usr/lib/jvm/java-6-openjdk-common/jre/lib/ext/pulse-java.jar!/../test/freenet/l10n/freenet.l10n.en.override.properties". When it works under Java 7 it's looking for "/home/steve/Documents/Coding/freenet/fred/build/main/../test/freenet/l10n/freenet.l10n.en.override.properties". Looks like the classLoaderDir cannot be assumed to be reasonable. I'll see what the other tests with files do.

bertm reviewed Nov 11, 2015
View reviewed changes

vwoodzell added 15 commits November 11, 2015 10:02

Add unit tests for BaseL10n

78161fe

Add some unit tests for BaseL10n.getString() and addL10nSubstitution().

Refactor a little to combine calls to addL10nSubstitutionInner()

7be8681

Avoid returning partially-populated node on l10n error

b1d86b7

Throw exceptions in BaseL10n.addL10nSubstitutionInner() when problems are encountered. Add wrapping tryAddL10nSubstitution() method which only applies changes to the node if no exception is thrown.

Always clone node before adding in addL10nSubstitutionInner()

68b84c6

Always clone node before adding to parent in BaseL10n.addL10nSubstitutionInner(), to avoid errors and for consistency.

Remove unused key parameter from addL10nSubstitutionInner()

486161a

Use fallback string on error in addL10nSubstitutionInner()

54c089a

If there's an error in the l10n string in BaseL10n.addL10nSubstitutionInner(), use the default string instead.

Add test for invalid l10n strings

9aa354a

Add test that tries to parse all l10n strings in order to find invalid ones.

Fix broken l10n substitution variables

e439cd8

Return key from addL10nSubstitution() if fallback fails

6adaacd

If we encounter errors in both the current language's value and the fallback value for a key, return the key itself from addL10nSubstitution().

Pass l10n parse exception into Logger.error()

a4e845c

Pass l10n parse exception into Logger.error() instead of extracting the message ourselves.

Separate BaseL10n.getFallbackString() from getDefaultString()

7d4b1c8

Separate l10n fallback logic from value handling

ad1fa81

Implement an iterator over the current language string, fallback language string, and literal key. Use in place of logic in getString(), getDefaultString(), and getL10nSubstitution().

Move exception handling out of BaseL10n.tryGetL10nSubstitution()

c61c334

Remove unneeded null check from BaseL10n.addL10nSubstitution()

2ca2f00

Improve method names and javadoc in BaseL10n

afc7d9c

vwoodzell force-pushed the l10n-test branch from c1a218b to afc7d9c Compare November 13, 2015 04:20

Copy l10n test properties in unit-build

fd1494d

Change buildfile to copy l10n test properties files into the unit test build dir.

Thynix merged commit fd1494d into hyphanet:next Dec 19, 2015

Thynix mentioned this pull request Dec 29, 2015

Setup Travis at https://travis-ci.org/freenet/fred/ #447

Merged

Thynix mentioned this pull request Dec 31, 2015

test: fix resource path on Java 6 #463

Merged

L10n tests and better error handling #421

L10n tests and better error handling #421

vwoodzell commented Nov 11, 2015

Bombe commented Nov 11, 2015

bertm Nov 11, 2015

bertm commented Nov 11, 2015

bertm commented Nov 11, 2015

vwoodzell commented Nov 13, 2015

Thynix commented Dec 7, 2015

vwoodzell commented Dec 7, 2015

Thynix commented Dec 29, 2015

Thynix commented Dec 31, 2015

L10n tests and better error handling #421

L10n tests and better error handling #421

Conversation

vwoodzell commented Nov 11, 2015

Bombe commented Nov 11, 2015

bertm Nov 11, 2015

Choose a reason for hiding this comment

bertm commented Nov 11, 2015

bertm commented Nov 11, 2015

vwoodzell commented Nov 13, 2015

Thynix commented Dec 7, 2015

vwoodzell commented Dec 7, 2015

Thynix commented Dec 29, 2015

Thynix commented Dec 31, 2015