March 18, 2013
On Monday, 18 March 2013 at 20:30:32 UTC, Jonathan M Davis wrote:
> On Monday, March 18, 2013 12:09:47 Walter Bright wrote:
>> On 3/18/2013 11:12 AM, Jonathan M Davis wrote:
>> > On Monday, March 18, 2013 17:34:12 Martin Nowak wrote:
>> >> Can someone look into std.file's unittests. They use 60% (32s/50s) of
>> >> the unittest RUNtime on my machine.
>> > 
>> > My first guess would be that you're running a different OS from Walter and
>> > that OS-specific code counts as not being run when you run it on a
>> > different OS.
>> Code that is statically compiled out is not counted as executable code by
>> the coverage analyzer.
>
> Then it was a bad guess. Good to know (and better behavior that way really).

Is it though? It basically means that if you don't instantiate a
template, it statically doesn't get compiled, so it is not
counted:

main.d:
//--------
        |void foo(T)()
        |{
        |   int i = 0;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |   ++i;
        |}
        |
        |void main()
        |{
       1|    int i;
        |}
main.d is 100% covered
//--------

main.d 100% covered my ass.

I also call BS on std.algorithm's 95% coverage. I looked at the
.lst. It is *roughly* covered, but not 95% covered.

We need to be able to differentiate code that is stripped out due
to "version" blocks (which legitimately doesn't count for the
current build), and code that simply didn't get compiled because
it WASN'T COVERED.
March 18, 2013
On 3/18/2013 2:09 PM, Jonas Drewsen wrote:
> Please note that std.net.curl needs to have PHOBOS_TEST_ALLOW_NET env variable
> defined or else it will early out on most unittests. I guess that will increase
> the coverage from 2 to something more sane.

Perhaps that should be a -version=TEST_ALLOW_NET setting rather than an environment variable.

March 18, 2013
On 03/18/2013 09:30 PM, Jonathan M Davis wrote:
>>> Can someone look into std.file's unittests. They use 60% (32s/50s) of
>>> > >>the unittest RUNtime on my machine.
>> > >
>> > >My first guess would be that you're running a different OS from Walter and
>> > >that OS-specific code counts as not being run when you run it on a
>> > >different OS.
>>Code that is statically compiled out is not counted as executable code by
>>the coverage analyzer.

Sorry for the misunderstanding, but I really meant runtime as in time.
It's great that we're striving for more unittest coverage but we should keep an eye on the time it takes to run them.
So I ran the executables with /bin/time and std.file accounts for 60% of the total runtime (not including compilation). Even though file IO is notoriously slow this seems excessive.

https://github.com/D-Programming-Language/phobos/pull/653

March 18, 2013
On Monday, 18 March 2013 at 22:06:55 UTC, Walter Bright wrote:
> On 3/18/2013 2:09 PM, Jonas Drewsen wrote:
>> Please note that std.net.curl needs to have PHOBOS_TEST_ALLOW_NET env variable
>> defined or else it will early out on most unittests. I guess that will increase
>> the coverage from 2 to something more sane.
>
> Perhaps that should be a -version=TEST_ALLOW_NET setting rather than an environment variable.

Maybe.. I did it like that so I wouldn't have to recompile everytime I needed to change that flag.
March 18, 2013
On Monday, March 18, 2013 22:13:36 monarch_dodra wrote:
> On Monday, 18 March 2013 at 20:30:32 UTC, Jonathan M Davis wrote:
> > On Monday, March 18, 2013 12:09:47 Walter Bright wrote:
> >> On 3/18/2013 11:12 AM, Jonathan M Davis wrote:
> >> > On Monday, March 18, 2013 17:34:12 Martin Nowak wrote:
> >> >> Can someone look into std.file's unittests. They use 60%
> >> >> (32s/50s) of
> >> >> the unittest RUNtime on my machine.
> >> > 
> >> > My first guess would be that you're running a different OS
> >> > from Walter and
> >> > that OS-specific code counts as not being run when you run
> >> > it on a
> >> > different OS.
> >> 
> >> Code that is statically compiled out is not counted as
> >> executable code by
> >> the coverage analyzer.
> > 
> > Then it was a bad guess. Good to know (and better behavior that
> > way really).
> 
> Is it though? It basically means that if you don't instantiate a template, it statically doesn't get compiled, so it is not counted:
> 
> main.d:
> //--------
> 
> |void foo(T)()
> |{
> |
> | int i = 0;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> | ++i;
> |
> |}
> |
> |void main()
> |{
> 
> 1| int i;
> 
> |}
> 
> main.d is 100% covered
> //--------
> 
> main.d 100% covered my ass.
> 
> I also call BS on std.algorithm's 95% coverage. I looked at the .lst. It is *roughly* covered, but not 95% covered.
> 
> We need to be able to differentiate code that is stripped out due to "version" blocks (which legitimately doesn't count for the current build), and code that simply didn't get compiled because it WASN'T COVERED.

Good point. But it _is_ valuable to not have unused version blocks not count negatively towards code coverage.

- Jonathan M Davis
March 18, 2013
On Monday, March 18, 2013 23:44:15 Martin Nowak wrote:
> On 03/18/2013 09:30 PM, Jonathan M Davis wrote:
> >>> Can someone look into std.file's unittests. They use 60% (32s/50s) of
> >>> 
> >>> > >>the unittest RUNtime on my machine.
> >> > >
> >> > >My first guess would be that you're running a different OS from Walter
> >> > >and
> >> > >that OS-specific code counts as not being run when you run it on a
> >> > >different OS.
> >>
> >>Code that is statically compiled out is not counted as executable code by the coverage analyzer.
> 
> Sorry for the misunderstanding, but I really meant runtime as in time.
> It's great that we're striving for more unittest coverage but we should
> keep an eye on the time it takes to run them.
> So I ran the executables with /bin/time and std.file accounts for 60% of
> the total runtime (not including compilation). Even though file IO is
> notoriously slow this seems excessive.
> 
> https://github.com/D-Programming-Language/phobos/pull/653

Yeah. The typical approach of testing each function individually is a bit expensive with std.file. It really should be refactored so that it does everything in only a few unittest blocks rather than a bunch of them so that it minimizes how much it repeats I/O operations. I've been meaning to go through it and do that but haven't gotten around to it yet. Thanks for reminding me though. I'd forgotten about it.

Still, 60% of the total runtime seems awfully high. I don't recall every getting the impression that anything like that was going on.

- Jonathan M Davis
March 18, 2013
On Monday, March 18, 2013 23:58:59 Jonas Drewsen wrote:
> On Monday, 18 March 2013 at 22:06:55 UTC, Walter Bright wrote:
> > On 3/18/2013 2:09 PM, Jonas Drewsen wrote:
> >> Please note that std.net.curl needs to have
> >> PHOBOS_TEST_ALLOW_NET env variable
> >> defined or else it will early out on most unittests. I guess
> >> that will increase
> >> the coverage from 2 to something more sane.
> > 
> > Perhaps that should be a -version=TEST_ALLOW_NET setting rather than an environment variable.
> 
> Maybe.. I did it like that so I wouldn't have to recompile everytime I needed to change that flag.

The normal thing to do with unit tests when you want to have them enabled on part of the time is to use a version block. And you have to recompile whenever you make changes anyway, so I wouldn't expect it to be a big deal in general.

- Jonathan M Davis
March 18, 2013
On 3/18/2013 4:08 PM, Jonathan M Davis wrote:
> The normal thing to do with unit tests when you want to have them enabled on
> part of the time is to use a version block. And you have to recompile whenever
> you make changes anyway, so I wouldn't expect it to be a big deal in general.

Also, a unittest executable is built and then run and then deleted. I see no point to keeping it around and running it in different ways, hence no real value in passing parameters to it via an environment variable.

March 19, 2013
A super critical part of tests that involve networking.. make sure you're pointing at resources that you own.  Pointing at even a popular site that you're sure can handle the load, is just rude.  The auto-tester currently runs the full build/test cycle a little over 1200 times a day these days, or roughly once a minute.  Multiply that by however many remote calls it makes.

And that's just the tester.. it doesn't include the ad-hoc test runs that people do.  Nor does it count whatever auto-testing is done for gdc or ldc.

On Mon, 18 Mar 2013, Jonas Drewsen wrote:

> Please note that std.net.curl needs to have PHOBOS_TEST_ALLOW_NET env variable defined or else it will early out on most unittests. I guess that will increase the coverage from 2 to something more sane.
> 
> -Jonas
> 
> >         $(DMD) -cov=2  -unittest -main -run std\net\curl.d
> 
March 19, 2013
On 3/18/2013 5:33 PM, Brad Roberts wrote:
> A super critical part of tests that involve networking.. make sure you're
> pointing at resources that you own.  Pointing at even a popular site that
> you're sure can handle the load, is just rude.  The auto-tester currently
> runs the full build/test cycle a little over 1200 times a day these days,
> or roughly once a minute.  Multiply that by however many remote calls it
> makes.
>
> And that's just the tester.. it doesn't include the ad-hoc test runs that
> people do.  Nor does it count whatever auto-testing is done for gdc or
> ldc.

I agree. We need to comport ourselves as good net citizens.