You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Danny Trebbien <dt...@gmail.com> on 2011/02/14 03:05:38 UTC
[PATCH] add a test of svn_subst_translate_string2() to the
subst_translate-test test suite (v. 2)
Attached is version 2 of the patch and log message.
>> + strncpy(orig_lc_all, setlocale(LC_ALL, NULL), sizeof orig_lc_all);
>
> sizeof() with parens please.
Fixed.
>> + if ((! setlocale(LC_ALL, "English.1252")) &&
>> + (! setlocale(LC_ALL, "German.1252")) &&
>> + (! setlocale(LC_ALL, "French.1252")) &&
>
> What are these three? Are they Windows syntax?
Yes.
>> + (! 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)
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.
>> +
>> + 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);
>> +
>> + SVN_TEST_ASSERT(setlocale(LC_ALL, orig_lc_all) != NULL);
>> + }
>
> So you only restore the locale if the test passed. How much does it
> matter?
It might cause problems in subsequent tests in the same test suite.
Using one of Brane's suggestions
(http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125792/focus=125795),
the new patch uses a wrapper function to ensure that the original
LC_ALL locale is restored before the test returns.
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +static svn_error_t *
>> test_repairing_svn_subst_translate_string2(apr_pool_t *pool)
>> {
>> {
>> @@ -171,6 +210,8 @@ struct svn_test_descriptor_t test_funcs[] =
>> SVN_TEST_NULL,
>> SVN_TEST_PASS2(test_svn_subst_translate_string2,
>> "test svn_subst_translate_string2()"),
>> + SVN_TEST_PASS2(test_svn_subst_translate_string2_null_encoding,
>> + "test svn_subst_translate_string2(), ENCODING = 0"),
>
> I know you are limited to 50 chars, but this "= 0" sacrifices
> readability IMHO. IMO, there are clearer alternatives, e.g.:
> "test svn_subst_translate_string2(encoding=NULL)"
I like that. Changed.
Re: [PATCH] add a test of svn_subst_translate_string2() to the
subst_translate-test test suite (v. 2)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Danny Trebbien wrote on Sun, Feb 13, 2011 at 18:05:38 -0800:
> >> + (! 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.
> >> + SVN_TEST_ASSERT(setlocale(LC_ALL, orig_lc_all) != NULL);
> >> + }
> >
> > So you only restore the locale if the test passed. How much does it
> > matter?
>
> It might cause problems in subsequent tests in the same test suite.
>
> Using one of Brane's suggestions
> (http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125792/focus=125795),
> the new patch uses a wrapper function to ensure that the original
> LC_ALL locale is restored before the test returns.
>
+1
> [[[
> 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.)
> * 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.
> (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.
> + /* 'Æ', 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