Skip to content

convert docstring examples to unittests [datetime]#4050

Closed
wilzbach wants to merge 2 commits intodlang:masterfrom
wilzbach:examples_to_unittest4
Closed

convert docstring examples to unittests [datetime]#4050
wilzbach wants to merge 2 commits intodlang:masterfrom
wilzbach:examples_to_unittest4

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Mar 4, 2016

Hey it seems like the datetime.d module is a bit messy - nevertheless I propose to convert its examples to unittests.

The PR is split in two parts:
a) "dumb" conversion from docstring to unittest (>100 tests, large commit)
b) small fixes to unittests (more interesting)

The unittests are displayed on my local machine, let's see what Doctester says ;-)

(Github did automatically closed #4048 once I rebased - sorry ...)

@wilzbach wilzbach changed the title Examples to unittest4 Examples to unittest [datetime] Mar 4, 2016
@wilzbach wilzbach changed the title Examples to unittest [datetime] convert docstring examples to unittests [datetime] Mar 4, 2016
@JackStouffer
Copy link
Contributor

Please refer to this comment #4042 (comment) about changing the examples in std.datetime

Also, in the future please break down your PRs into smaller and separate PRs, as it makes it much easier to review. For example, the diff in this PR is so large that I can't review it because Github refuses to show it.

@wilzbach
Copy link
Contributor Author

wilzbach commented Mar 4, 2016

Please refer to this comment #4042 (comment) about changing the examples in std.datetime

It was wondering why it should be problematic to put unittest block inside of struct template and whether the documentation and tests would still work on std.datetime. It turns out they do. Maybe I am just a bit too motivated/juvenile;-)

Also, in the future please break down your PRs into smaller and separate PRs, as it makes it much easier to review. For example, the diff in this PR is so large that I can't review it because Github refuses to show it.

I didn't know about this feature lack in github earlier - it was just one file and felt silly to split it up. Sorry!

@JackStouffer where to proceed from here - if @jmdavis

a bunch of the examples in std.datetime shouldn't be converted either, because the functions in question are member functions on templated types, and it's problematic to put unittest blocks inside of templates

is really valid - this PR is useless for you?

At least you now know that

  1. auto func = (in Date date) -> needs to be changed to auto func = (in Date date) const
  2. dur!"years"(3)) doesn't work anymore - in its context (1 occurrence) it can be replaced with dur!"days"(1096)

@jmdavis
Copy link
Member

jmdavis commented Mar 4, 2016

I didn't know about this feature lack in github earlier - it was just one file and felt silly to split it up. Sorry!

Unfortunately, it's a problem that we run into frequently with std.datetime, and it's on my todo list to split it up into a package.

is really valid - this PR is useless for you?

Okay. You should pretty much never put a unittest block inside of a template. If you do, it will be compiled in with every instantiation of that template (including when 3rd parties instantiate it). In addition, if the template is not instantiated in tests outside of the template, then the template will not be instantiated as part of the unit tests and will not be tested. If you have a function inside of a template, then its tests need to go outside that template, which means that you can't use ddoc-ed unittest blocks. The examples need to be copy-pasted into unittest blocks outside of the template (or the code in the unittest blocks need to be copy-pasted into the ddoc). And yes, that sucks, but that's life right now. http://wiki.dlang.org/DIP82 was proposed to fix it, but unless/until it is, please do not put any examples in ddoc-ed unittest blocks if they're inside of a template.

In the case of the *Interval types in std.datetime, there are unittest blocks which correspond to each function after the declaration for that type (each unittest block should be labeled with which function it goes with). Each should have a section marked with something like

//Verify Examples.

and the examples from that function should be in there. If you find that the actual example in the documentation and those marked tests do not match, then please create a PR to fix them, but please do not try and move all of those examples into unittest blocks inside of the templated types. It will result in longer build times (including for folks importing Phobos and using those types, not just for the Phobos unit tests), and in this case, all of those examples should already be in unittest blocks and tested. It's just that because we can't currently use ddoc-ed unittest blocks without compiling them into every instantiation, we run the same risks with the examples being wrong that we ran before we could ddoc unittest blocks.

So, while I appreciate your enthusiasm, this is one case where ddoc-ed unit tests just don't work right now, and I'm closing this. Please create a PR with fixes for any bugs you found in the examples, and I'd love to merge that. Hopefully, DIP 82 will be accepted and implemented at some point, but until it is, we just can't reasonably take advantage of ddoc-ed unittest blocks here, much as I would love to (ddoc examples aside, simply having to put the tests after the type instead of right after the functions that they test is incredibly annoying, and not being able to use ddoc-ed unit tests makes it that much worse).

@wilzbach
Copy link
Contributor Author

wilzbach commented Mar 5, 2016

Update: I saw that in Slice (from ndslice) unittests are used in the template - they use this pattern:
static if (doUnittest)

https://github.com/D-Programming-Language/phobos/blob/master/std/experimental/ndslice/slice.d#L947

@jmdavis
Copy link
Member

jmdavis commented Mar 6, 2016

The result is that the unittest blocks get compiled in with only one instantiation of the template, but it still means that someone who instantiates Slice with those arguments is going to be compiling those unit tests into their program when compiling with -unittest, and if the code ever got refactored in a way that the template was never instantiated with those specific arguments in unit tests outside of the template, then those tests would not be compiled in and run as part of Phobos' unit test build. So, it's an attempt to work around the problem and does mitigate it, but it doesn't solve it. What we really need is something like what DIP 82 proposes.

@wilzbach wilzbach mentioned this pull request Mar 6, 2016
@wilzbach wilzbach deleted the examples_to_unittest4 branch February 19, 2017 23:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants