On 4/3/13, Jacob Carlborg <doob@me.com> wrote:
> The problem is having the unit tests in the same file. Yes, I know, most of you love it, I don't.

One thing I noticed is that having unittests in separate files can catch issues with template mixins.

If you have any private or protected functions that are used by a mixin template, the mixin template will not compile once the user tries to use it in his own code.

There are workarounds, of course, like putting functions inside of the template. But the point still stands that you need to also test the library externally.

Another thing local unittests don't test are symbol clashes. If a user imports lib.a and lib.b from your library, he probably doesn't expect to get symbol clashes.

In fact Phobos has had symbol clashes before, and we're working on getting rid of them (e.g. through deprecation stages). But if Phobos also had external test-cases then we could have avoided symbol clashes to begin with.

On 4/3/13, Andrej Mitrovic <andrej.mitrovich@gmail.com> wrote:
> If you have any private or protected functions

I meant private or package.

> One thing I noticed is that having unittests in separate files can catch issues with template mixins.

I wonder if there's a way to mitigate that problem with a language feature. Perhaps marking the unittest as 'extern' would make the unittest only have access to public symbols in the module.

That way you never get into the situation where testing something from within a unittest seems to work, but completely forgetting that you're calling a private or package function.

On 2013-04-03 05:03, Jonathan M Davis wrote:

> I very much doubt that you could do that unless you specifically formatted the
> code to take up as few lines as possible and didn't count the unit tests or
> documentation in that line count. Otherwise, you couldn't do anything even
> close to what std.datetime does in that few lines. Sure, some functionality
> could be stripped, but you'd end up with something that did a lot less if it
> were that small. The unit tests and documentation do make it seem like a lot
> more code than it is, since they take up well over half the file (probably
> 3/4), but you'd definitely lose functionality with that few lines of code, and
> you'd end up with something very poor IMHO if those 2000 lines included the
> documentation and unit tests. You'd either end up with something that was very
> bare-bones and/or something which was poorly tested, and given how easy it is
> to screw up some of those date/time calculations, having only a few tests
> would be a very bad idea.

Since he wrote "2000 lines for all functionality", I don't think he included unit tests or docs/comments.

> std.datetime's unit tests do need some refactoring (some of which I've done,
> but there's still a fair bit of work to do there), which will definitely reduce
> the number of LOC that they take up, but I don't agree at all with considering
> the unit tests as part of the LOC of file when discussing keeping LOC to a
> minimum. And while it's good to avoid repetitive unit tests, I'd much rather
> have repetitive unit tests which are thorough than short ones which aren't. I
> find your focus on trying to keep unit tests to a minimum to be disturbing and
> likely to lead to poorly tested code.

> If anything, we need to be more thorough, not less. That doesn't mean that the
> tests need to look like what std.datetime has (particularly since I
> purposefully avoided loops and other more complicated constructs when I wrote
> them originally in order to make them as simple and as far from error-prone as
> possible), but unit tests need to be thorough, and while we're getting better,
> Phobos' unit tests frequently aren't thorough enough (particularly in
> std.range and std.algorithm when it comes to testing a variety of range
> types). Too many of them just test a few cases to make sure that the most
> obvious stuff works rather than making sure they test corner cases and whatnot.
>
> - Jonathan M Davis
>

I actually prefer to have repetitive unit tests and not using loops to make it clear what they actually do. Here's an example from our code base, in Ruby:

describe "Swedish" do
  subject { build(:address) { |a| a.country_id = Country::SWEDEN } }

  it { should validate_postal_code(12345) }
  it { should validate_postal_code(85412) }

  it { should_not validate_postal_code(123) }
  it { should_not validate_postal_code(123456) }

  it { should_not validate_postal_code("05412") }
  it { should_not validate_postal_code("fooba") }
end

describe "Finnish" do
  subject { build(:address) { |a| a.country_id = Country::FINLAND } }

  it { should validate_postal_code(12345) }
  it { should validate_postal_code(12354) }
  it { should validate_postal_code(41588) }

  it { should validate_postal_code("00123") }
  it { should validate_postal_code("01588") }
  it { should validate_postal_code("00000") }

  it { should_not validate_postal_code(1234) }
  it { should_not validate_postal_code(123456) }
  it { should_not validate_postal_code("fooba") }
end

It could be written less repetitive, like this:

postal_codes = {
  Country::SWEDEN => {
    valid: [12345, 85412],
    invalid: [123, 123456, "05412", "fooba"]
  },

  Country::FINLAND => {
    valid: [12345, 12354, 41588],
    invalid: ["00123", "01588", "00000", 1234, 123456, "fooba"]
  }
}

postal_codes.each do |country_id, postal_codes|
  describe c.english_name do
    subject { build(:address) { |a| a.country_id = country_id } }

    postal_codes[:valid].each do |postal_code|
      it { should validate_postal_code(postal_code) }
    end

    postal_codes[:invalid].each do |postal_code|
      it { should_not validate_postal_code(postal_code) }
    end
  end
end

But I don't think that looks any better. I think it's much worse.

-- 
/Jacob Carlborg

On 2013-04-03 08:45, Andrej Mitrovic wrote:

> One thing I noticed is that having unittests in separate files can
> catch issues with template mixins.
>
> If you have any private or protected functions that are used by a
> mixin template, the mixin template will not compile once the user
> tries to use it in his own code.
>
> There are workarounds, of course, like putting functions inside of the
> template. But the point still stands that you need to also test the
> library externally.

I didn't think about that.

> Another thing local unittests don't test are symbol clashes. If a user
> imports lib.a and lib.b from your library, he probably doesn't expect
> to get symbol clashes.

Most likely not, but there's nothing wrong with it. We do have modules for a reason. It's fairly easy do solve for the user if the issue comes up. If there are some common names that always clash, then there are some problems.

> In fact Phobos has had symbol clashes before, and we're working on
> getting rid of them (e.g. through deprecation stages). But if Phobos
> also had external test-cases then we could have avoided symbol clashes
> to begin with.

I don't know if that's something unit tests should explicitly test for.

-- 
/Jacob Carlborg

On Wednesday, 3 April 2013 at 02:44:15 UTC, Andrei Alexandrescu wrote:
> If we did datetime all over again, I'd give a budget of 2000 lines for all functionality. I bet the solution would be better.

I think you are massively underestimating the complexity and subtleties of dates and time.

For comparison, min and max in std.algorithm come to nearly 200 lines on their own, and their unittests are hopelessly lacking. Things like min(uint.min, int.max) are not tested, even though there's specific code to handle them. To suggest that date and time handling is a mere 10x more complex than min/max is a bit naive in my opinion.

03-Apr-2013 19:55, Peter Alexander пишет:
> On Wednesday, 3 April 2013 at 02:44:15 UTC, Andrei Alexandrescu wrote:
>> If we did datetime all over again, I'd give a budget of 2000 lines for
>> all functionality. I bet the solution would be better.
>
> I think you are massively underestimating the complexity and subtleties
> of dates and time.
>

+1

> For comparison, min and max in std.algorithm come to nearly 200 lines on
> their own, and their unittests are hopelessly lacking. Things like
> min(uint.min, int.max) are not tested, even though there's specific code
> to handle them. To suggest that date and time handling is a mere 10x
> more complex than min/max is a bit naive in my opinion.
>


-- 
Dmitry Olshansky

On 4/3/13 11:55 AM, Peter Alexander wrote:
> On Wednesday, 3 April 2013 at 02:44:15 UTC, Andrei Alexandrescu wrote:
>> If we did datetime all over again, I'd give a budget of 2000 lines for
>> all functionality. I bet the solution would be better.
>
> I think you are massively underestimating the complexity and subtleties
> of dates and time.

May as well. I recall before I approved std.datetime I looked at the implementation sizes of similar functionality in other languages; they were all rather bulky, but std.datetime was at the high end of the range.

> For comparison, min and max in std.algorithm come to nearly 200 lines on
> their own, and their unittests are hopelessly lacking. Things like
> min(uint.min, int.max) are not tested, even though there's specific code
> to handle them. To suggest that date and time handling is a mere 10x
> more complex than min/max is a bit naive in my opinion.

To put things in perspective, std.datetime has 34K lines, whereas std.algorithm has under 12K lines. The entire std/ has 191K lines. I'd be hard pressed to assess that that high proportion is justified.

Say we set out to fit std.datetime in e.g. 20K lines without loss in functionality or testing, which I'd find more reasonable. I think the result would force overall better engineering of the entire thing (and in particular better use of data structures) - constraints may be liberating.


Andrei

On 4/2/13 11:03 PM, Jonathan M Davis wrote:
> I
> find your focus on trying to keep unit tests to a minimum to be disturbing and
> likely to lead to poorly tested code.

Well that's quite the assumption.

Andrei

On 4/3/13 2:53 AM, Jacob Carlborg wrote:
> On 2013-04-03 05:03, Jonathan M Davis wrote:
> I actually prefer to have repetitive unit tests and not using loops to
> make it clear what they actually do. Here's an example from our code
> base, in Ruby:
>
> describe "Swedish" do
> subject { build(:address) { |a| a.country_id = Country::SWEDEN } }
>
> it { should validate_postal_code(12345) }
> it { should validate_postal_code(85412) }
>
> it { should_not validate_postal_code(123) }
> it { should_not validate_postal_code(123456) }
>
> it { should_not validate_postal_code("05412") }
> it { should_not validate_postal_code("fooba") }
> end
>
> describe "Finnish" do
> subject { build(:address) { |a| a.country_id = Country::FINLAND } }
>
> it { should validate_postal_code(12345) }
> it { should validate_postal_code(12354) }
> it { should validate_postal_code(41588) }
>
> it { should validate_postal_code("00123") }
> it { should validate_postal_code("01588") }
> it { should validate_postal_code("00000") }
>
> it { should_not validate_postal_code(1234) }
> it { should_not validate_postal_code(123456) }
> it { should_not validate_postal_code("fooba") }
> end
>
> It could be written less repetitive, like this:
>
> postal_codes = {
> Country::SWEDEN => {
> valid: [12345, 85412],
> invalid: [123, 123456, "05412", "fooba"]
> },
>
> Country::FINLAND => {
> valid: [12345, 12354, 41588],
> invalid: ["00123", "01588", "00000", 1234, 123456, "fooba"]
> }
> }
>
> postal_codes.each do |country_id, postal_codes|
> describe c.english_name do
> subject { build(:address) { |a| a.country_id = country_id } }
>
> postal_codes[:valid].each do |postal_code|
> it { should validate_postal_code(postal_code) }
> end
>
> postal_codes[:invalid].each do |postal_code|
> it { should_not validate_postal_code(postal_code) }
> end
> end
> end
>
> But I don't think that looks any better. I think it's much worse.

The way I see it, the first is terrible and the second asks for better focus on a data-driven approach.

Andrei

On Tuesday, April 02, 2013 20:41:23 Walter Bright wrote:
> On 4/2/2013 8:03 PM, Jonathan M Davis wrote:
> > Too many of them just test a few cases to make sure that the most obvious stuff works rather than making sure they test corner cases and whatnot.
> Currently, the datetime unittest coverage is 95%. Some of the 0 cases suggest low hanging fruit.
> 
> Despite what I just said, datetime has one of the highest unittest coverages of any phobos module. Pretty much all of the phobos module unittest coverage testing indicates more work is needed.
> 
> Minor perf improvement: the order of the tests in yearIsLeapYear() should be reversed, especially since signed divide is a very slow operation, and it is called 20 million times by the unittests!!!

Yes. That's one of the things that I need to improve. std.datetime has a lot of tests, so it needs to do a better job of ordering stuff within unittest blocks in a manner which minimizes their cost. They need to be thorough, but they should also efficient, or the tests will end up taking too long (which is why it doensn't do a lot of testing with exceptions right now, since they slow the tests down considerably).

- Jonathan M Davis