On 2013-08-08 10:25, David Mason wrote:
> In zanata-war/src/test/java/org/zanata/tmx/TMXParserTest.java:
> 
>> +      assertThat(tm.getMetadata().size(), greaterThan(0));
>> +      assertThat(tm.getMetadata().get(TMMetadataType.TMX14), notNullValue());
>> +
>> +      // Translation Units
>> +      for(TransMemoryUnit tu : tm.getTranslationUnits())
>> +      {
>> +         assertThat(tu.getTransUnitVariants().size(), greaterThan(0));
>> +      }
>> +   }
>> +
>> +   @Test
>> +   @org.junit.Test
>> +   public void parseDubiousTMX() throws Exception
>> +   {
>> +      // Create a TM
>> +      TransMemory tm = createTMFromFile("/tmx/dubious-tm-with-underscores.tmx");
> 
> The comment says the same thing as the code, but is less thorough. I'd
> rather the comment told me something I can't tell from the code, such as
> exactly what is supposed to be dubious about the file used.

Just the underscores.  According the TMX spec, the locales should be
BCP47 (hyphens, no underscores), but real-world files seem to use
underscores quite frequently, so we have to support them.  Eventually, I
would like to return a list of warnings to the user when uploading
dubious files like this one.

> I would move the declaration of |expectedLocales| up here (it is part of
> the given scenario for this test), and name it to clarify why that is
> expected (i.e. that these are the locales for which there are tuvs in
> the file that was used). Source locale in the file could similarly be
> extracted to an explanatory variable in this section.

I thinks tests are easier to read if they have the expectations down the
bottom where practical (ie Given, When, Then).


> 
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/zanata/zanata-server/pull/78/files#r5648164>.
> 


-- 
Sean Flanigan

Senior Software Engineer
Engineering - Internationalisation
Red Hat