ishii****@nttda*****
ishii****@nttda*****
2015年 8月 27日 (木) 10:22:10 JST
> On Mon, Aug 24, 2015 at 10:58 PM, Fujii Masao <masao****@gmail*****> wrote: > > On Mon, Aug 24, 2015 at 4:33 PM, <sawad****@nttda*****> wrote: > >> > >>> -----Original Message----- > >>> From: ludia****@lists***** > >>> [mailto:ludia****@lists*****] On Behalf Of > >>> ishii****@nttda***** > >>> Sent: Monday, August 24, 2015 3:09 PM > >>> To: ludia****@lists***** > >>> Subject: Re: [Ludiafuncs-hackers] Add regression tests > >>> > >>> Thank you for review. > >>> I updated the patch. > >>> > >>> > > Changes from the last time: > >>> > > ・Delete unnecessary "CREATE USER" > >>> > > (We use "CREATE USER" only when we create normal user.) > >>> > > >>> > You can use SET ROLE and RESET ROLE command in order to do > >>> > permission test, instead of adding textporter-close test. > >>> > >>> I fixed to use SET ROLE instead of textporter-close test. > >>> > >>> > > expected/ludia_funcs.out:789: trailing whitespace. > >>> > > +\c euc_db > >>> > > expected/ludia_funcs.out:795: trailing whitespace. > >>> > > +\c contrib_regression > >>> > > sql/ludia_funcs.sql:176: trailing whitespace. > >>> > > +\c euc_db > >>> > > sql/ludia_funcs.sql:180: trailing whitespace. > >>> > > +\c contrib_regression > >>> > > >>> > There are some unnecessary trailing whitespaces in > >>> > 000_ludia_funcs_regress.patch as follows. > >>> > >>> I fixed to delete unnecessary trailing whitespaces. > >>> > >>> > > $ patch -d. -p1 < > >>> > > >>> > > ../patch/ludia_funcs_regress_patch/001_ludia_funcs_regress_debug_o > >>> > pt > >>> > io > >>> > > n.patch > >>> > > patching file Makefile > >>> > > Hunk #1 FAILED at 33. > >>> > > 1 out of 1 hunk FAILED -- saving rejects to file Makefile.rej > >>> > I could not apply 001_ludia_funcs_regress_debug_option.patch > >>> > successfully. > >>> > > >>> > It looks like both file debug_option.patch and textporter.patch > >>> > have wrong prefix number each other. > >>> > debug_options patch should be applied after textporter patch applied. > >>> > >>> I fixed the number. > >>> ・000_ludia_funcs_regress.patch > > Pushed a part of this patch. > > > +-- pgs2seninfo() checks > > +select * from pgs2seninfo('test'); > > > > What is the purpose of this test? You'd like to check that > > pgs2seninfo(text) is NOT created unexpectedly, IOW, you'd like to > > validate the definition of the function? If yes, something like "\df+ > pgs2seninfo()" might be better. > > > > +SELECT * FROM pgs2norm(); > > +SELECT * FROM pgs2norm('abc','あいう'); > > > > Same as above. > > I excluded these tests from the commit because I'm not sure if it's worth > adding these tests. > > > +-- ludia_funcs.norm_cache_limit validation checks SET > > +ludia_funcs.norm_cache_limit TO 2147483647; SET > > +ludia_funcs.norm_cache_limit TO -1; SET ludia_funcs.norm_cache_limit > > +TO '10kB'; SET ludia_funcs.norm_cache_limit TO 'test'; SET > > +ludia_funcs.norm_cache_limit TO -2; SET ludia_funcs.norm_cache_limit > > +TO 2147483648; > > > > I'm not sure whether this tests are worthwhile or not, but if we want > > to validate the definition of the GUC, I think it's better to check > > the pg_settings entry of the GUC parameter. > > Same as above. > > > +CREATE DATABASE euc_db ENCODING 'EUC_JP' TEMPLATE template0; > > > > I think it's better to use more regression test *special* database > > name rather than euc_db. For example, contrib_regression_euc, > > ludia_funcs_regression_euc, etc. > > I renamed the database name to regtest_ludia_funcs_eucjp. And then, I > changed the test so that it drops the regtest_ludia_funcs_eucjp database > before testing the case where the encoding is not UTF8. Because I'd like > to avoid executing "\c contrib_regression" in the test. I'm not sure if > it's guaranteed that contrib_regression is *always* used as the database > for the regression test. > So "\c contrib_regression" might fail in some environment. BTW, there is > no code like "\c contrib_regression" in PostgreSQL's regression tests. Thank you for pushing the patch. I agree with you. -- Ayumi Ishii NTT DATA CORPORATION
Fork