Fujii Masao
masao****@gmail*****
2015年 8月 27日 (木) 01:56:01 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_opt >>> > 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. Regards, -- Fujii Masao
Fork