Обсуждение: Email to hackers for test coverage

On Sat, Aug 24, 2019 at 11:23:32PM +0800, movead.li@highgo.ca wrote:
> Thanks and I just do it, it is
> https://commitfest.postgresql.org/24/2258/

Your patch has forgotten to update the alternate output in
float4-misrounded-input.out.
--
Michael

>Your patch has forgotten to update the alternate output in

>float4-misrounded-input.out.

Thanks for your remind, I have modified the patch and now it is 

'regression_20190826.patch' in attachment, and I have done a successful

test on Cygwin.

--

Movead

On Mon, Aug 26, 2019 at 05:10:59PM +0800, movead.li@highgo.ca wrote:
> Thanks for your remind, I have modified the patch and now it is
> 'regression_20190826.patch' in attachment, and I have done a successful
> test on Cygwin.

There is a section in float4.sql which deals with overflow and
underflow, so wouldn't it be better to move the tests there?  You
could just trigger the failures with that:
=# insert into float4_tbl values ('-10e-70'::float8);
ERROR:  22003: value out of range: underflow
LOCATION:  check_float4_val, float.h:145
=# insert into float4_tbl values ('-10e70'::float8);
ERROR:  22003: value out of range: overflow
LOCATION:  check_float4_val, float.h:140

I would also test all four patterns: 10e70, 10e-70, -10e70, -10e-70.

For the numeric part, this improves the case of
ApplySortAbbrevFullComparator() where both values are not NULL.  Could
things be done so as the other code paths are fully covered?  One
INSERT is fine by the way to add the extra coverage.
--
Michael

On Tue, 27 Aug 2019 14:07:48 +0800 michael@paquier.xyz wrote:

> There is a section in float4.sql which deals with overflow and

> underflow, so wouldn't it be better to move the tests there?  You
> could just trigger the failures with that:
> =# insert into float4_tbl values ('-10e-70'::float8);
> ERROR:  22003: value out of range: underflow
> LOCATION:  check_float4_val, float.h:145
> =# insert into float4_tbl values ('-10e70'::float8);
> ERROR:  22003: value out of range: overflow
> LOCATION:  check_float4_val, float.h:140

> I would also test all four patterns: 10e70, 10e-70, -10e70, -10e-70.

 

I think your way is much better, so I change the patch and it is  

'regression_20190827.patch' now.

> For the numeric part, this improves the case of

> ApplySortAbbrevFullComparator() where both values are not NULL.  Could

> things be done so as the other code paths are fully covered?  One

> INSERT is fine by the way to add the extra coverage.

There are code lines related to NULL values in

ApplySortAbbrevFullComparator(), but I think the code lines for

comparing a NULL and a NOT NULL can be never reached, because it is

handled in ApplySortComparator() which is called before

ApplySortAbbrevFullComparator(). So I think it is no use to INSERT

a NULL value.

--

Movead

On Tue, Aug 27, 2019 at 03:57:20PM +0800, movead.li@highgo.ca wrote:
> I think your way is much better, so I change the patch and it is
> 'regression_20190827.patch' now.

Thanks for the new patch, I have committed the part for float4.

> There are code lines related to NULL values in
> ApplySortAbbrevFullComparator(), but I think the code lines for
> comparing a NULL and a NOT NULL can be never reached, because it is
> handled in ApplySortComparator() which is called before
> ApplySortAbbrevFullComparator(). So I think it is no use to INSERT
> a NULL value.

But I am not sold to that part yet, for three reasons:
- numeric is not a test suite designed for sorting, and hijacking it
to do so it not a good approach.
- it would be good to get coverage for the two extra code paths while
we are on it with NULL datums.
- There is no need for two INSERT queries, I am getting the same
coverage with only one.

Please note that I have not looked in details where we could put that,
but perhaps Robert and Peter G who worked on 4ea51cd to add support
for abbreviated keys have some ideas, so I am adding them in CC for
input.
--
Michael

On Wed, 28 Aug 2019 11:30:23 +0800 michael@paquier.xyz wrote

>- numeric is not a test suite designed for sorting, and hijacking it

>to do so it not a good approach.

Absolutely agreement. We can wait for repling from

Robert and Peter G.

>- it would be good to get coverage for the two extra code paths while

>we are on it with NULL datums..

The remained untested line in ApplySortComparator() can be never

reached I think, and I have explained it on last mail. 

Or I don't fully understand what you mean?

 

>- There is no need for two INSERT queries, I am getting the same

>coverage with only one.

Yes It is my mistake,  the key point is 'ORDER BY n1 DESC' not the 

'INSERT'. Infact it can run the code line without any of the two INSERT.

--

Movead

On 2019-08-22 11:46, movead.li@highgo.ca wrote:
> *1. src/include/utils/float.h:140*
> 
> Analyze: 
> This is an error report line when converting a big float8 value
> which a float4 can not storage to float4.
> 
> Test case:
> Add a test case as below in file float4.sql:
> select float4(1234567890123456789012345678901234567890::float8);

> +-- Add test case for float4() type fonversion function

Check spelling

> *2. src/include/utils/float.h:145*
> 
> Analyze:
> This is an error report line when converting a small float8 value
> which a float4 can not storage to float4.
> 
> Test case:
> Add a test case as below in file float4.sql:
> select float4(0.0000000000000000000000000000000000000000000001::float8);
> 
> *3.src/include/utils/sortsupport.h:264*
> 
> Analyze:
> It is reverse sorting for the data type that has abbreviated for
> sort, for example macaddr, uuid, numeric, network and I choose
> numeric to do it.
> 
> Test cast:
> Add a test case as below in file numeric.sql:
> INSERT INTO num_input_test(n1) values('99999999999999999999999999.998');
> INSERT INTO num_input_test(n1) values('99999999999999999999999999.997');
> SELECT * FROM num_input_test ORDER BY n1 DESC;

>  INSERT INTO num_input_test(n1) VALUES ('        nan');
> +INSERT INTO num_input_test(n1) values('99999999999999999999999999.998');
> +INSERT INTO num_input_test(n1) values('99999999999999999999999999.997');

Make spaces and capitalization match surrounding code.

> Result and patch
> 
> By adding the test cases, the test coverage of  float.h increased from
> 97.7% to 100% and sortsupport.h increased from 76.7% to 80.0%.

That's fine, but I suggest that if you really want to make an impact in
test coverage, look to increase function coverage rather than line
coverage.  Or look for files that are not covered at all.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Aug 27, 2019 at 8:30 PM Michael Paquier <michael@paquier.xyz> wrote:
> Please note that I have not looked in details where we could put that,
> but perhaps Robert and Peter G who worked on 4ea51cd to add support
> for abbreviated keys have some ideas, so I am adding them in CC for
> input.

Movead is correct -- the NULL handling within
ApplySortAbbrevFullComparator() cannot actually be used currently. I
wouldn't change anything about the code, though, since it's useful to
defensively handle NULLs.

-- 
Peter Geoghegan

On 2019-08-29 00:43, peter.eisentraut@2ndquadrant.com wrote:

 

 

> Make spaces and capitalization match surrounding code..

>That's fine, but I suggest that if you really want to make an impact in

>test coverage, look to increase function coverage rather than line

>coverage.  Or look for files that are not covered at all.

 

Thanks for pointing all the things, I will reconsider my way on

code coverage work.

--

Movead

On Wed, Aug 28, 2019 at 11:27:15AM -0700, Peter Geoghegan wrote:
> Movead is correct -- the NULL handling within
> ApplySortAbbrevFullComparator() cannot actually be used currently. I
> wouldn't change anything about the code, though, since it's useful to
> defensively handle NULLs.

No objections with this line of thoughts.  Thanks, Peter.  Please note
that I have marked the original patch as committed in the CF app.  If
there are tests to improve the coverage, let's do that on a new
thread.  I am still not sure where I would put tests dedicated to
abbreviated keys, but let's sort out that if necessary later.
--
Michael