dicts in doctests

[Erik Bray]

I think this question has come up before, but I don't know that
there's been a really satisfactory solution.

Many of the doctests have dicts as their output (or worse, contained
embedded in their output). This can fail from time to time since dict
element order is not guaranteed. This has been particularly bad in
the conversion to Python 3 since there are significant changes in the
dict implementation itself (in particular Python 3.6 preserves the
insertion order of dictionary keys, which is very nice).

Up 'til now I've been making little test fixes here and there by,
instead of outputting dict object directly, doing something like
sorted((k, v) for k, v in d.items(), key=lambda x: str(x[0]) and then
comparing the sorted list of (key, value) tuples (it also becomes
necessary to convert the keys to str or something similar, since a lot
of types that previously had an ordering in Python 2 are no longer
orderable on Python 3 (e.g. str and int).

Obviously this is ugly, and might be impractical to do in every single case.

Did anyone ever think up a better solution to this?

I'm thinking of maybe going so far as to parsing dict literals out of
the test output and re-ordering them (this can be done reasonably well
using the ast module). But is it worth it?

Thanks,
Erik

[Jeroen Demeyer]

On 2017-12-06 15:49, Erik Bray wrote:
> I think this question has come up before, but I don't know that
> there's been a really satisfactory solution.
>
> Many of the doctests have dicts as their output (or worse, contained
> embedded in their output). This can fail from time to time since dict
> element order is not guaranteed.

In doctests, the output of dicts is sorted. So I guess you run into
trouble only when

(A) The dict is embedded in some deeper structure: no general solution,
a case-by-case fix is needed

(B) The sorting is different in Python 2 and Python 3: this is a general
consequence of the __cmp__/__richcmp__ differences. One important ticket
is https://trac.sagemath.org/ticket/22029

(C) The keys cannot be sorted: fixed by
https://github.com/ipython/ipython/pull/10767 which is not in Sage yet.

I would not try to fix cases (B) and (C) in doctests because they will
be fixed at some point anyway.

[Erik Bray]

Aha, thanks for the summary of the current state of affairs. I didn't
realize plain dicts were already sorted in their repr--that is good.

In that case probably most of the examples I've encountered are indeed
with more nested data structures. I'd have to double-check but that
sounds right for the examples I can think of off the top of my head.

I think then for (A) the most general solution would be the one I
suggested--of basically scanning for dict literals, parsing them, and
reordering them. This isn't too hard in most cases; the only concern
might be if there are any cases of output that looks exactly like a
dict literal but for some reason isn't, and shouldn't be messed with.
I don't know that there are any cases like that though.

[Maarten Derickx]

I strongly dislike the idea making the doctesting framework more complicated by parsing the output like this. I think this should only be done as a last resort, since it makes the doctesting framework itself more error prone, and if implemented incorrectly might even silently let real error slip by. In more complicated data structures it is probably more a sign of the complicated data structure not pretty printing itself in a nice way (i.e. failing to pretty print the dicts it depends on) then anything else.

p.s. according to https://trac.sagemath.org/ticket/23586 it will help a lot if we upgrade ipython to a version where https://github.com/ipython/ipython/pull/10767 is merged before switching to py3. I do not know wether this has been done already on some other ticket.

[Michael Orlitzky]

On 12/06/2017 09:49 AM, Erik Bray wrote:
>
> Did anyone ever think up a better solution to this?
>

Whatever you do, you wind up with a big pile of dict output in the
middle of your test case. So (where possible) you might as well use that
space to define a new dict containing the expected value. For example,

>>> actual = some_computation()
>>> expected = { 1: "one",
... 2: "two",
... 9: "nine" }
>>> actual == expected
True

That's harder to do when the dict is buried in some other data
structure, but maybe we shouldn't be testing the exact string
representation of some big conglomerate in the first place -- do we
actually care what it is? Instead, we should test only what we care about.

Regardless, I don't think the framework should make it look like you can
rely on dicts being sorted when you can't. Users should be able to run
the EXAMPLES and see what we say they'll see.

[Kwankyu Lee]

Users should be able to run the EXAMPLES and see what we say they'll see.

+1 

[Erik Bray]

That is a good thing and a bad thing about so many of the tests
doubling as "examples" in the documentation (especially for 'public'
methods to the extent that there is such a thing). You want them to
actually function nicely as examples of how a user should use an
interface and what a user should expect to see. Going into
contortions in order to make it function reliably as a test can be at
odds with that.

If we're talking one dict it's not much of a problem. The main
problem seems to be with more complex objects that contain dicts
nested in their reprs. And you do sometimes want to test the repr
string itself too (that, however, could be in the form of a TEST not
an EXAMPLE).

[Erik Bray]

Another workaround that's so obvious I smacked myself on the head is
that for many cases, particularly objects that have a small dict in
their representation, is to simply change the __repr__ so that its
dict is always displayed sorted. If the order doesn't matter anyways
that it doesn't hurt to impose an order at least for the __repr__. I
doubt there are many cases where this should have any performance
impact either.

[Maarten Derickx]

I think this is indeed the way to go. In fact this is what I was trying to say with my remark "In more complicated data structures it is probably more a sign of the complicated data structure not pretty printing itself in a nice way (i.e. failing to pretty print the dicts it depends on) then anything else.". Which I admit is a bit cryptic, but essentially suggests your workaround because dicts are sorted when pretty printed.

[Michael Orlitzky]

On 12/12/2017 09:30 AM, Erik Bray wrote:
>
> Another workaround that's so obvious I smacked myself on the head is
> that for many cases, particularly objects that have a small dict in
> their representation, is to simply change the __repr__ so that its
> dict is always displayed sorted. If the order doesn't matter anyways
> that it doesn't hurt to impose an order at least for the __repr__. I
> doubt there are many cases where this should have any performance
> impact either.
>

It makes the user experience worse (i.e. slower) for the sole benefit of
a test suite used only non-interactively by developers. That's my slight
moral objection, but sometimes it may still be the best option.

[Emmanuel Charpentier]

I think that printing the result of bool(effective_result==expected_result) is sufficient for testing purposes, and has the advantage of testing mathematical equality, not only string equality.

You may want to be more explicit when failing : you may try (something like) :

res=bool(effective_result==expected_result)
if not res: res="Expected : {} \nEffective : {}".format(expected_result, effective_result)
print res

That way, you can expect "True" as the result f the doctest, and still get hints about the failure if it fails.

What do you think ?

--
Emmanuel Charpentier

[Erik Bray]

In most cases here we're only talking about repr-ing small dicts.
This has no user-visible impact.

[Erik Bray]

Yeah, I see now that pprint (and its underlying pformat) print dicts
sorted. I've already started using that for some cases and it seems
to get the job done. My only annoyance is that it's very oriented
toward terminal printing, and there's not a great way to control how
lines are wrapped (or force it not to be multi-line) short of setting
the width argument very large. So I've resorted to doing `'
'.join(pformat(my_dict).splitlines())` which gets the job done but is
a little ugly.

[John Cremona]

Indeed it is -- fine for a test but not in an "Example".

For a test/example which computes a dict which we expect to be expected_value (in some sorting) why not put

sage: actual_value = ....

sage: actual_value # random order

sage: assert actual_value == <expected_value>

so we see the display, the test doesn't care about how it looks, but we also test that it is correct without cluttering up the documentation.

 

--
You received this message because you are subscribed to the Google Groups "sage-devel" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sage-devel+unsubscribe@googlegroups.com.
To post to this group, send email to sage-...@googlegroups.com.
Visit this group at https://groups.google.com/group/sage-devel.
For more options, visit https://groups.google.com/d/optout.

[Erik Bray]

To be clear, the code I wrote above specifically pertains to dicts
that are embedded in the __repr__ of some object. In other words,
that code goes in the __repr__ implementation, not the example/test.
This ensures that such objects are always displayed predictably.

[Nils Bruin]

On Thursday, December 14, 2017 at 3:29:32 AM UTC-8, Erik Bray wrote:

To be clear, the code I wrote above specifically pertains to dicts
that are embedded in the __repr__ of some object.  In other words,
that code goes in the __repr__ implementation, not the example/test.
This ensures that such objects are always displayed predictably.

-1 to letting a basic "__repr__" of any object depend on pprint. "__repr__" can end up in error objects which may get caught and never printed. The effort of pprint is not appropriate for those cases.

We can easily deal with random orders in documentation by marking the printing of the dict with "#random" and testing the result for correctness in a more semantically meaningful way.

[Erik Bray]

On Thu, Dec 14, 2017 at 10:02 PM, Nils Bruin <nbr...@sfu.ca> wrote:
> On Thursday, December 14, 2017 at 3:29:32 AM UTC-8, Erik Bray wrote:
>>
>>
>> To be clear, the code I wrote above specifically pertains to dicts
>> that are embedded in the __repr__ of some object. In other words,
>> that code goes in the __repr__ implementation, not the example/test.
>> This ensures that such objects are always displayed predictably.
>
>
> -1 to letting a basic "__repr__" of any object depend on pprint. "__repr__"
> can end up in error objects which may get caught and never printed. The
> effort of pprint is not appropriate for those cases.

"the effort of pprint"--is trivial compared to unwinding the stack, etc.

> We can easily deal with random orders in documentation by marking the
> printing of the dict with "#random" and testing the result for correctness
> in a more semantically meaningful way.

Nah.

[Erik Bray]

On Fri, Dec 15, 2017 at 12:11 PM, Erik Bray <erik....@gmail.com> wrote:
> On Thu, Dec 14, 2017 at 10:02 PM, Nils Bruin <nbr...@sfu.ca> wrote:
>> On Thursday, December 14, 2017 at 3:29:32 AM UTC-8, Erik Bray wrote:
>>>
>>>
>>> To be clear, the code I wrote above specifically pertains to dicts
>>> that are embedded in the __repr__ of some object. In other words,
>>> that code goes in the __repr__ implementation, not the example/test.
>>> This ensures that such objects are always displayed predictably.
>>
>>
>> -1 to letting a basic "__repr__" of any object depend on pprint. "__repr__"
>> can end up in error objects which may get caught and never printed. The
>> effort of pprint is not appropriate for those cases.
>
> "the effort of pprint"--is trivial compared to unwinding the stack, etc.

Sorry, let me just clarify again--I'm specifically talking about a
small (AFAIK) handful of cases involving small (typically < 10
element) dicts that happen to be embedded in the reprs of a few
classes (and often internal ones that would rarely be repr'd by a
user). For anything more significant where it could actually matter
it becomes a separate question.