Enable runnable examples - remove the last modules from the blacklist by wilzbach · Pull Request #5607 · dlang/phobos

wilzbach · 2017-07-12T20:04:23Z

After quite a bit of work, we are heading for an end :)
This PR removes the last five modules from the runnable blacklist, s.t. with 2.076 all unittests are runnable

(FYI: the effort started in #4943)

For some modules, I wasn't sure sure on the best approach and I will mark these bits individually.

wilzbach · 2017-07-12T20:07:08Z

std/experimental/allocator/common.d

 */
 @safe @nogc nothrow pure
-package size_t roundUpToMultipleOf(size_t s, uint base)
+size_t roundUpToMultipleOf(size_t s, uint base)


This exposes roundUpToMultipleOf to the docs, it's used in many of the building blocks and the documented examples in quantizer needs it. An alternative would be to use a more simplistic Quantizer than:

alias MyAlloc = Quantizer!( FreeTree!GCAllocator, n => n.roundUpToMultipleOf(n <= 16_384 ? 64 : 4096)); ``

I made it package-visible because it's in the wrong place. Once we expose it we'll need to do it forever. It should be shuttled to the right place. I'd say put it in std.numeric. Other possible candidates: std.conv, std.math.

Also, it's much less generic than it could and should be because there wasn't a need. Once we expose it as a public primitive, the requirements change. It probably needs to accept any two unsigned types.

wilzbach · 2017-07-12T20:08:00Z

std/stdio.d

        static import std.file;

-        auto testFile = testFilename();
+        auto testFile = std.file.deleteme();


Not a huge fan of this, but deleteme is the "defacto" standard within Phobos and used in many other public examples.

wilzbach · 2017-07-12T20:11:15Z

std/traits.d


-///
+//
 @safe unittest


Access to private members is not possible with getSymbolsByUDA - the test only works it's in the same module.
Bug report

I suggest moving out the bad parts of the unit test rather than hiding the entire test.

I read the issue 17643 I agree with @CyberShadow that we should leave this in a private unittest, until we reach a consensus about whether access to private members should be allowed. (I think one should be able to get the attributes of private members at least in the same module, though that's not what this unittest tested..)

CyberShadow · 2017-07-12T21:45:20Z

@wilzbach wilzbach requested review from andralex, CyberShadow, ibuclaw, klickverbot, MartinNowak, schveiguy, and ZombineDev as code owners 2 hours ago

That is excessive.

I hoped this feature would be a little smarter and disable itself when it would otherwise ping more than 3-5 people.

CyberShadow · 2017-07-12T21:46:55Z

The std.stdio changes are fine by me. I guess we never decided if we want to make testFilename or something like that public, which unfortunately means using an undocumented symbol in our documentation examples.

andralex

We need to make sure published functions get due treatment. How about for now you just inline the rounding code in examples therefore making roundUpToMultipleTo unnecessary?

andralex · 2017-07-12T23:31:41Z

std/experimental/allocator/common.d

 */
 @safe @nogc nothrow pure
-package size_t roundUpToMultipleOf(size_t s, uint base)
+size_t roundUpToMultipleOf(size_t s, uint base)


I made it package-visible because it's in the wrong place. Once we expose it we'll need to do it forever. It should be shuttled to the right place. I'd say put it in std.numeric. Other possible candidates: std.conv, std.math.

andralex · 2017-07-12T23:32:46Z

std/experimental/allocator/common.d

 */
 @safe @nogc nothrow pure
-package size_t roundUpToMultipleOf(size_t s, uint base)
+size_t roundUpToMultipleOf(size_t s, uint base)


Also, it's much less generic than it could and should be because there wasn't a need. Once we expose it as a public primitive, the requirements change. It probably needs to accept any two unsigned types.

wilzbach · 2017-07-13T00:11:02Z

That is excessive.
I hoped this feature would be a little smarter and disable itself when it would otherwise ping more than 3-5 people.

I thought the same ... according to their docs:

Order is important; the last matching pattern takes the most precedence.

Inlined.

ibuclaw · 2017-07-13T21:06:10Z

std/math.d


-    // check if values are equal to 19 decimal digits of precision
-    assert(equalsDigit(mantissa * pow(2.0L, cast(real) exp), 123.456L, 19));
+    assert(approxEqual(mantissa * pow(2.0L, cast(real) exp), 123.456L));


I'd have to check this on a target that demanded it.

You don't need to, I moved the test to an internal unittest. This change is just needed because approxEqual isn't a public symbol and would error when run online (or wherever the user tries to run the example).

Yes, but does it pass on ARM? Or any other real==double target for that matter?

If equalsDigit and approxEqual are sufficiently the same, then there's no problem. :)

If equalsDigit and approxEqual are sufficiently the same, then there's no problem. :)

approxEqual (used for the public examples) is much weaker, than equalsDigit. approxEqual checks by a difference of 1e-5 by default, whereas the equalsDigit tests here are by a precision of 1e-19.

ibuclaw · 2017-07-13T21:09:08Z

std/math.d

 }

 ///
+@system unittest


Actually, why are you testing with both approxEqual() and equalsDigit(19)?

equalsDigit is private, that's why I copied the test to a separate unittest and inserted approxEqual instead

dlang-bot · 2017-07-17T18:11:30Z

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Description
✓	16984	Make more modules runnable on dlang.org

wilzbach · 2017-07-17T18:15:28Z

The std.stdio changes are fine by me. I guess we never decided if we want to make testFilename or something like that public, which unfortunately means using an undocumented symbol in our documentation examples.

Well it's better than not having public tests or having public ones that aren't working for the tests.
Btw see also: #5400

So is anything blocking this then?

@CyberShadow we really have an issue with our CIs. Sometimes even everything is red, despite having "good" changes:

CyberShadow · 2017-07-17T22:01:49Z

we really have an issue with our CIs

Yep.

Turns out continuous integration is hard. Who would have guessed? :)

wilzbach · 2017-07-20T03:01:18Z

Turns out continuous integration is hard. Who would have guessed? :)

Okay I tried rebasing for this one - doesn't seem to work either:

At least before "auto-tester" was green, so how do we proceed here?

wilzbach · 2017-09-14T00:06:03Z

As the GH issues have been resolved, this should be finally good to go.
Ping @CyberShadow :)

CyberShadow · 2017-09-14T02:36:03Z

std/traits.d

 }

-///
+//


Why are we undocumenting useful example blocks?

Because hasRawAliasing is a private symbol -> https://github.com/dlang/phobos/blob/v2.076.0/std/traits.d#L2661

Oh, OK. So this was an "example" for a private symbol.

CyberShadow · 2017-09-14T02:37:29Z

std/traits.d


-///
+//
 @safe unittest


I suggest moving out the bad parts of the unit test rather than hiding the entire test.

wilzbach · 2017-10-05T19:38:37Z

Access to private members is not possible with getSymbolsByUDA - the test only works it's in the same module.
Bug report
I suggest moving out the bad parts of the unit test rather than hiding the entire test.

Done - the other public unittest that I removed in std.traits was from a private symbol. Anything else or can be get this in before 2.077? Would be great because then we can finally enable "runnable examples" for all modules on dlang.org

wilzbach · 2017-10-05T20:08:37Z

Looks like CircleCi is also finally green again :)

wilzbach · 2017-10-07T15:25:14Z

so?
Ping @CyberShadow @ZombineDev

PetarKirov

Apart from what I've commented on , everything looks good.

PetarKirov · 2017-10-06T14:58:25Z

std/experimental/allocator/building_blocks/free_list.d


-        ///
-        @safe unittest
+        @system unittest


I wonder why it worked with @safe before?

Because it was never executed, see the comment about unittest blocks in templates before. By moving the unittest to the module level, this unittest was executed for the first time...

Quite unfortunate. Thanks for the clarification.

PetarKirov · 2017-10-06T15:04:15Z

std/experimental/allocator/building_blocks/free_list.d

-            a.setBounds(64, 128); // equivalent
-            assert(a.max == 128);
-            assert(a.min == 64);
-        }


What's the reason for moving the unit test to module level? Is it because the test extractor cannot handle unit tests for member functions? I don't like this change as now the example won't show immediately after the function, but on rather different place (especially with the ddox layout).

A unittest in a template will be part of every instantiation, they shouldn't have been there in the first place.See DIP82 for a more detailed description of the problem.
Also note that the unittest will still be associated with its struct.

This can be worked around via static if (doUnittest) (see e.g. https://github.com/libmir/mir-algorithm/blob/e590f7f46058588f1ceb08672c92bd6248863e18/source/mir/ndslice/slice.d#L737).
Essentially you need to make sure that doUnittest is true for a single set of template arguments and that you have a template instance with those arguments (e.g. used in a private unittest).

(If you don't want to deal with this now, we can leave it for later.)

This can be worked around via static if (doUnittest)

Considering that the test didn't compile until now (in case you didn't notice, the moved out version is different - it doesn't set the bounds twice as this is prohibited), I was merely trying to get things to work, s.t. we can finally move forward with the runnable examples ;-)
Regarding the static if (doUnittest): I am aware of this pattern (even the old std.experimental.ndslice used this), but AFAIK that has been the only occurrence of this pattern in Phobos.
Anyways, there are still plenty of unittests in templated structs/classes, so I would strongly prefer if such a general cleanup is done in a later PR...

Fair enough

PetarKirov · 2017-10-06T15:04:26Z

std/experimental/allocator/building_blocks/free_list.d

-            assert(a.approxMaxLength  == 1024);
-            a.approxMaxLength = 1;
-            assert(a.approxMaxLength  == 1);
-        }


PetarKirov · 2017-10-06T15:14:20Z

std/traits.d


-///
+//
 @safe unittest


I read the issue 17643 I agree with @CyberShadow that we should leave this in a private unittest, until we reach a consensus about whether access to private members should be allowed. (I think one should be able to get the attributes of private members at least in the same module, though that's not what this unittest tested..)

wilzbach added the Infrastructure:automation label Jul 12, 2017

wilzbach requested review from CyberShadow, MartinNowak, PetarKirov, andralex, dnadlinger, ibuclaw and schveiguy as code owners July 12, 2017 20:04

wilzbach commented Jul 12, 2017

View reviewed changes

andralex previously requested changes Jul 12, 2017

View reviewed changes

wilzbach force-pushed the enable_runnable_examples branch from bb4921f to 92e319f Compare July 13, 2017 00:14

ibuclaw reviewed Jul 13, 2017

View reviewed changes

wilzbach closed this Jul 17, 2017

wilzbach reopened this Jul 17, 2017

dlang-bot added the Severity:Enhancement label Jul 17, 2017

wilzbach force-pushed the enable_runnable_examples branch from 92e319f to 6f48f83 Compare July 20, 2017 03:00

wilzbach force-pushed the enable_runnable_examples branch from 6f48f83 to 56e9e6d Compare September 14, 2017 00:05

CyberShadow reviewed Sep 14, 2017

View reviewed changes

wilzbach added 3 commits October 5, 2017 21:30

Issue 16984 - Make std.math runnable

2e96d06

Issue 16984 - Make std.stdio runnable

c2d03bf

Issue 16984 - Make std.traits runnable

3cc6300

wilzbach force-pushed the enable_runnable_examples branch from 56e9e6d to fce9f42 Compare October 5, 2017 19:36

wilzbach added 4 commits October 5, 2017 21:55

Issue 16984 - Make stdx.allocator.building_blocks.quantizer runnable

ff6c0f1

Issue 16984 - Make stdx.allocator.building_blocks.free_list runnable

5a20f69

Fix Issue 16984 - Remove runnable blacklist

d661120

Remove invalid private access by getSymbolsByUDA from unittest

41de215

wilzbach force-pushed the enable_runnable_examples branch from fce9f42 to 41de215 Compare October 5, 2017 19:55

PetarKirov reviewed Oct 7, 2017

View reviewed changes

PetarKirov approved these changes Oct 7, 2017

View reviewed changes

PetarKirov added the Merge:auto-merge label Oct 7, 2017

dlang-bot merged commit 42a1ee5 into dlang:master Oct 7, 2017

wilzbach deleted the enable_runnable_examples branch October 10, 2017 20:25

wilzbach mentioned this pull request Nov 2, 2017

[2.077] Remove runnable examples blacklist dlang/dlang.org#1924

Merged

               }
-              ///
+              //

Uh oh!

Conversation

wilzbach commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jul 12, 2017

Uh oh!

CyberShadow commented Jul 12, 2017

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlang-bot commented Jul 17, 2017

Bugzilla references

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

wilzbach commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Oct 5, 2017

Uh oh!

wilzbach commented Oct 5, 2017

Uh oh!

wilzbach commented Oct 7, 2017

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach Jul 12, 2017 •

edited

Loading

wilzbach commented Jul 20, 2017 •

edited

Loading

wilzbach Oct 7, 2017 •

edited

Loading