[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] add a test of svn_subst_translate_string2() to the subst_translate-test test suite (v. 3)

From: Danny Trebbien <dtrebbien_at_gmail.com>
Date: Sun, 20 Feb 2011 09:58:07 -0800

Attached is version 3 of the patch and log message.

>> >> +        (! setlocale(LC_ALL, "en_US.ISO-8859-1")) &&
>> >> +        (! setlocale(LC_ALL, "en_GB.ISO-8859-1")) &&
>> >> +        (! setlocale(LC_ALL, "de_DE.ISO-8859-1")))
>> >> +      return svn_error_createf(SVN_ERR_TEST_SKIPPED, NULL, "None of the locales English.1252, German.1252, French.1252, en_US.ISO-8859-1, en_GB.ISO-8859-1, and de_DE.ISO-8859-1 are installed.");
>> >
>> > The error message is too verbose, and doesn't fit within 80 columns.
>>
>> Currently the error message is not displayed if none of the locales
>> are available;  the output is simply:
>>
>> SKIP:  lt-subst_translate-test 2: test
>> svn_subst_translate_string2(encoding = NULL)
>>
>
> Oddly, the message isn't displayed even with --verbose.  But if you
> write an error message, I think it's best to assume it'll be read by
> someone. (or else don't write it at all)
>
>> I wrapped it to 80 characters, but I would rather not change the
>> verbosity of the text because I like how it indicates what was tried
>> and also that setlocale() fails because a locale is not installed.
>>
>
> +1 on leaving the latter detail in.  However, the error message just
> duplicates the code; those details should be conveyed by err->file and
> err->line, not by err->message.
>
> So, sorry, but I'm not yet convinced that listing all the locales in the
> log message is a good idea.

Okay. I changed it to "Tried %d locales, but none are installed."

>> [[[
>> Add a test of svn_subst_translate_string2() to the subst_translate test suite.
>>
>> As discussed at:
>>   http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125782
>>
>
> Message-ID, please, for forward compatibility.  (The apache.org and
> gmane.org archives allow you to embed the Message-ID in the URL; or just
> give it separately.)

Okay. The Message-ID is
<AANLkTimo=NpF_FqB+PBPkeifXjbnsORGYkZ5npCAxyVz_at_mail.gmail.com>, which
is provided separately because I like how the Gmane link displays the
whole discussion thread.

>> * subversion/tests/libsvn_subr/subst_translate-test.c
>>   (test_svn_subst_translate_string2_null_encoding_helper): New function. It is
>>     the core of the new svn_subst_translate_string2_null_encoding test.
>>   (test_svn_subst_translate_string2_null_encoding): New test that tests
>>     svn_subst_translate_string2() with ENCODING set to NULL. It wraps
>>     test_svn_subst_translate_string2_null_encoding_helper() to ensure that the
>>     system-default character encoding is set to either CP-1252 or ISO-8859-1
>>     before test_svn_subst_translate_string2_null_encoding_helper() is called,
>>     later restoring the original system-default character encoding.
>
> This amount of information ought to be a docstring, not a log message.

Fixed.

>>   (test_funcs): Add test_svn_subst_translate_string2_null_encoding().
>> ]]]
>
>> @@ -99,7 +102,64 @@ test_svn_subst_translate_string2(apr_pool_t *pool)
>>    return SVN_NO_ERROR;
>>  }
>>
>> +/* The body of the svn_subst_translate_string2_null_encoding test. It should
>> +   only be called by test_svn_subst_translate_string2_null_encoding(), as this
>> +   code assumes that the process locale has been changed to a locale that uses
>> +   either CP-1252 or ISO-8859-1 for the default narrow string encoding. */
>>  static svn_error_t *
>> +test_svn_subst_translate_string2_null_encoding_helper(apr_pool_t *pool)
>> +{
>> +  {
>> +    svn_string_t *new_value = NULL;
>> +    svn_boolean_t translated_to_utf8 = FALSE;
>> +    svn_boolean_t translated_line_endings = TRUE;
>
> Why did you initialize these two variables?  The API will always set
> them for you, so you shouldn't initialize them.

It's part of the test to check that the API sets them.

>> +    /* 'Æ', which is 0xc6 in both ISO-8859-1 and Windows-1252 */
>> +    svn_string_t *source_string = svn_string_create("\xc6", pool);
>> +
>> +    SVN_ERR(svn_subst_translate_string2(&new_value, &translated_to_utf8,
>> +                                        &translated_line_endings,
>> +                                        source_string, NULL, FALSE,
>> +                                        pool, pool));
>> +    SVN_TEST_STRING_ASSERT(new_value->data, "\xc3\x86");
>> +    SVN_TEST_ASSERT(translated_to_utf8 == TRUE);
>> +    SVN_TEST_ASSERT(translated_line_endings == FALSE);
>> +  }
>> +
>> +  return SVN_NO_ERROR;
>> +}
>> +
>> +/* Test that when ENCODING is NULL, the system-default language encoding is used. */
>> +static svn_error_t *
>> +test_svn_subst_translate_string2_null_encoding(apr_pool_t *pool)
>> +{
>
> +1 on the rest; looks good.
>
> Thanks,
>
> Daniel

Received on 2011-02-20 18:58:45 CET

This is an archived mail posted to the Subversion Dev mailing list.