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

L10n tests and better error handling #421

Merged
merged 16 commits into from Dec 19, 2015

Conversation

Projects
None yet
4 participants
@vwoodzell
Copy link
Contributor

vwoodzell commented Nov 11, 2015

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

This comment has been minimized.

Copy link
Member

Bombe commented Nov 11, 2015

Looks good.

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

This comment has been minimized.

@bertm

bertm Nov 11, 2015

Member

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

* null if an error is encountered.
*/
private List<HTMLNode> tryGetL10nSubstitution(String key, String value,
String[] patterns, HTMLNode[] values) {

This comment has been minimized.

@bertm

bertm Nov 11, 2015

Member

Looks like this method only performs substitution. There's no need to pass the key here.

return null;
}
return tempNode.getChildren();
}

/**
* @see #addL10nSubstitution(HTMLNode, String, String[], HTMLNode[])

This comment has been minimized.

@bertm

bertm Nov 11, 2015

Member

This javadoc does not make sense. addL10nSubstitution does key lookup, addL10nSubstitutionInner does not.

String value = getString(key);
addL10nSubstitutionInner(node, key, value, patterns, values);
List<HTMLNode> newContent = getL10nSubstitution(key, patterns, values);
if (newContent != null) {

This comment has been minimized.

@bertm

bertm Nov 11, 2015

Member

newContent is never null by definition of getL10nSubstitution.

@bertm

This comment has been minimized.

Copy link
Member

bertm commented Nov 11, 2015

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

This comment has been minimized.

Copy link
Member

bertm commented Nov 11, 2015

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

vwoodzell added some commits Nov 7, 2015

Add unit tests for BaseL10n
Add some unit tests for BaseL10n.getString() and addL10nSubstitution().
Avoid returning partially-populated node on l10n error
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()
Always clone node before adding to parent in
BaseL10n.addL10nSubstitutionInner(), to avoid errors and for
consistency.
Use fallback string on error in addL10nSubstitutionInner()
If there's an error in the l10n string in
BaseL10n.addL10nSubstitutionInner(), use the default string instead.
Add test for invalid l10n strings
Add test that tries to parse all l10n strings in order to find invalid
ones.
Return key from addL10nSubstitution() if fallback fails
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()
Pass l10n parse exception into Logger.error() instead of extracting the
message ourselves.
Separate l10n fallback logic from value handling
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 vwoodzell force-pushed the vwoodzell:l10n-test branch from c1a218b to afc7d9c Nov 13, 2015

@vwoodzell

This comment has been minimized.

Copy link
Contributor

vwoodzell commented Nov 13, 2015

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

@Thynix

This comment has been minimized.

Copy link
Member

Thynix commented Dec 7, 2015

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] 
Copy l10n test properties in unit-build
Change buildfile to copy l10n test properties files into the unit test
build dir.
@vwoodzell

This comment has been minimized.

Copy link
Contributor

vwoodzell commented Dec 7, 2015

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 Thynix merged commit fd1494d into freenet:next Dec 19, 2015

@Thynix

This comment has been minimized.

Copy link
Member

Thynix commented Dec 29, 2015

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

This comment has been minimized.

Copy link
Member

Thynix commented Dec 31, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment