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
