Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We need tests for example code #11340

Open
twisted-trac opened this issue Jul 15, 2003 · 24 comments
Open

We need tests for example code #11340

twisted-trac opened this issue Jul 15, 2003 · 24 comments

Comments

@twisted-trac
Copy link

spiv's avatar spiv reported
Trac ID trac#84
Type enhancement
Created 2003-07-15 13:04:29Z
Branch https://github.com/twisted/twisted/tree/testable-examples-84
Searchable metadata
trac-id__84 84
type__enhancement enhancement
reporter__spiv spiv
priority__high high
milestone__ 
branch__branches_testable_examples_84 branches/testable-examples-84
branch_author__rwall rwall
status__new new
resolution__ 
component__core core
keywords__documentation documentation
time__1058274269000000 1058274269000000
changetime__1364318206000000 1364318206000000
version__ 
owner__rwall rwall
cc__exarkun cc__spiv cc__jml cc__binjured cc__rwall
@twisted-trac
Copy link
Author

spiv's avatar spiv commented
#!html
<pre>
We occasionally break example code with new releases,
because we don't have any tests that verify that the example
code works.  I'm not sure how to fix this, but it would be
nice to figure out a way to do it.

</pre>

@twisted-trac
Copy link
Author

jml's avatar @jml commented
#!html
<pre>
Perhaps just running pychecker over the examples would be a
good start?

</pre>

@twisted-trac
Copy link
Author

spiv's avatar spiv commented
#!html
<pre>
Hmm, yeah, that could help -- but see also #86.  Twisted
currently causes lots of spurious warnings from pychecker,
so it'd be hard to easily tell whether an example is broken,
or is just importing code that triggers false positives.

</pre>

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun commented
#!html
<pre>
doctests would be the best way to deal with this problem, I think.
</pre>

@twisted-trac
Copy link
Author

puzzlement's avatar @puzzlement set owner to edsuom

@twisted-trac
Copy link
Author

glyph's avatar @glyph set owner to @khorn

Here's hoping that you can do something with this shortly after the Sphinx conversion...

@twisted-trac
Copy link
Author

glyph's avatar @glyph commented

This code might serve as a good example, or at least, an example: http://divmod.org/trac/browser/trunk/Nevow/nevow/test/test_howtolistings.py

@twisted-trac
Copy link
Author

binjured's avatar binjured commented

Just to clarify, we are now looking to replace broken/stale code with tested-first examples. I don't think a ticket that addresses tests for all sample code is very useful (or likely to be maintained properly) in the first place, so it'd be nice if we could close this.

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun commented

We should resolve this ticket with a plan about how to write testable examples. Then, indeed, there can be many more tickets for replacing (which may not actually mean deleting and rewriting in all cases, existing examples that can be reformed should be) existing examples with tested equivalents.

@twisted-trac
Copy link
Author

khorn's avatar @khorn commented

Even better than a plan would be a plan plus an actual example of a test harness for a particular example or set of examples.

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun commented

Well, let me update the link glyph posted, then:

http://bazaar.launchpad.net/~glyph/divmod.org/trunk/view/head:/Nevow/nevow/test/test_howtolistings.py

@twisted-trac
Copy link
Author

Automation's avatar Automation removed owner

@twisted-trac
Copy link
Author

wallrj's avatar @wallrj commented

In ticket:5596:names-examples-tests-5596.patch Download

  • Added tests for consistent USAGE documentation and command line error reporting
  • Added tests for standard python shebang line and that the script is executable.

I copied and modified the Nevow example testing mixin mentioned above.

Maybe the same tests can be re-used on other examples.

@twisted-trac
Copy link
Author

wallrj's avatar @wallrj commented

(In [37383]) Branching to 'testable-examples-84'

@twisted-trac
Copy link
Author

wallrj's avatar @wallrj commented

(In [37384]) some new twisted.names example tests for discussion. refs #11340

@twisted-trac
Copy link
Author

wallrj's avatar @wallrj commented

Replying to exarkun:

We should resolve this ticket with a plan about how to write testable examples. Then, indeed, there can be many more tickets for replacing (which may not actually mean deleting and rewriting in all cases, existing examples that can be reformed should be) existing examples with tested equivalents.

I've started a new Wiki page with the outlines of a plan.

There is also a branch where I added some further tests for the twisted.names examples.

  • log:branches/testable-examples-84

Before I go any further I'd appreciate a review of the plan.

Tell me which parts of the plan make sense, and I will create corresponding tickets where the discussion can continue.

Other parts of the plan I will refine or ditch, based on feedback.

@twisted-trac
Copy link
Author

tomprince's avatar @tomprince commented

That is a very well thought out document. Thanks.

  1. "example code should be a complete working example suitable for copying and pasting and running by the reader"
  • Should it also be cross platform? Or should there be platform specic examples? eg can an example refer to /etc/hosts
    I think it will depend on the examples. systemd examples will clearly be platform specific. Perhaps ideally, examples of cross-platform functionality will be cross-platform. But, clearly, that shouldn't be a reason for removing an example. And, I think it doesn't hurt to have some platform-specific examples of cross-platform functionality, in addition to cross-platform examples.
  1. "example code should be short"
  • How short? In terms of lines? Or in terms of the number of top level fuctions and classes? Perhaps an example of a too long example.

I'm not sure if there is a good single measure. Those are both reasonable hints. Perhaps some measure of complexity, or extraneous complexity would be good.

  1. "example code should be commented very extensively"
  • Does this include docstrings?
    Yes
  • Does this include epytext style API docs in docstrings?
    Maybe? I think this might depend on a case-by-case basis.
    Can too many comments be distracting in a simple example?
    Probably.
  1. "example code should exhibit 'best practice'"
  • Should we identify
    yes
    remove examples which refer to old deprecated APIs?
    no. As glyph has commented elsewhere, we don't want to remove documentation or tests for deprecated things until that functionality is actually removed. People may still encounter those APIs, and having examples to learn how they are to be used is useful for that purpose.

What is best practice? I guess most Twisted core devs will agree on most things, but are there any controversial practices eg inlineCallbacks?
I think currently this is largely an oral tradition (couting IRC as oral :)). What there is is encoded in the coding-standard and other policy documents. It would be good to capture more of it.

Can examples ever use, or suggest using external libraries? Eg treq, Nevow, ampoule, tx??? etc. If those external libraries are considered best practice? I think its quite common for people to be directed to better external alternatives eg treq vs Agent or wokel?? vs twisted.words??

<tomprince> dreid: Fetching webpages is the point of the client side of twisted web. Saying, "if you want to get webpages, go use treq", while currently the best answer and potentially a reasonable stance to take, isn't the stance twisted wants to take in the long run, in my opinion.
<glyph> tomprince: hi, yes. treq is license-compatible though, if you want to move things between them you can.
<tomprince       On the other hand, I'm not sure if the requst like api that treq has, is the api that twisted wants to expose.
<tomprince>       I think a similar argument could be made for parts of klein.
<tomprince>       glyph: Sure. I don't want to step on someones toes, though.
<glyph>   tomprince: right, so treq and klein have a more rapid release cycle and fewer compatibility guarantees so they can experiment and evolve
<glyph>   tomprince: when it looks right it can happily move into twisted :)
<dreid>   tomprince: I don't disagree that a higher level API for an HTTP client should be in Twisted.  I just have no immediate plans to propose treq as that higher level API.
<glyph>   tomprince: You've got basically the right idea but there's no need to rush.
<glyph>   I mean, no need to lag either; if you think you have an appropriate high-level request API taking ideas and possibly code from treq, by all means submit it for review.
<glyph>   But I don't think there's any need to worry if things continue as they are.

@twisted-trac
Copy link
Author

tomprince's avatar @tomprince commented

Some comments on the code in the branch (which should eventually be attached to a separate ticket:

  1. positionalArgCount needs an @ivar.

    • But, I'm not sure this can be made generic. Some commands may take an arbitrary number of options and some may do further checks on the options.
  2. I like the unit tests. But, we should figure out some way to test that fakeGetHostByName behaves enough like getHostByName to serve as a fake. Calling getHostByName is the point of the example, so by patching it out, we avoid testing what is essentially the most important part of the example.

    Buildbot has [https://github.com/buildbot/buildbot/blob/master/master/buildbot/test/util/interfaces.py something].

@twisted-trac
Copy link
Author

tomprince's avatar @tomprince commented

Some other comments:

  • doc/historic/ should be append only, so we don't want to run linter's on it.
  • find isn't portable. It would be better to extend twistedchcker to run on arbitrary directories, or something.
  • We also want to check code from the howtos (some of which might be incomplete snippets). Arguably, we probably want to turn a bunch of the examples into howtos at some point.

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun commented

We also want to check code from the howtos (some of which might be incomplete snippets). Arguably, we probably want to turn a bunch of the examples into howtos at some point.

I'd like to emphasize this. I think we actually want to turn all the examples into howtos. An example with narration is vastly more educational than an example in a void.

@twisted-trac
Copy link
Author

khorn's avatar @khorn commented

Replying to exarkun:

We also want to check code from the howtos (some of which might be incomplete snippets). Arguably, we probably want to turn a bunch of the examples into howtos at some point.

I'd like to emphasize this. I think we actually want to turn all the examples into howtos. An example with narration is vastly more educational than an example in a void.

One way of handling this would be to move all example code out of the howto documents into separate python files, for ease of testing. Then pull relevant parts of the code (functions, classes, line number ranges, etc) into the howto documents for rendering into HTML. This might make things a bit more difficult for HOWTO authors, however, as they would have to be looking at two different files when writing up their docs.

Lore can only do this by line number, though IIRC there is a ticket for other types of includes. (aha - #11558)

Sphinx can do this, but obviously we haven't switched over to Sphinx at this point in time.

In any case, I'm really only talking about long-term plans here. Nothing that should delay or derail this ticket.

@twisted-trac
Copy link
Author

exarkun's avatar @exarkun commented

Lots of existing howtos already do this. I don't think there's much (if any) deficiency in the current tool support.

@twisted-trac
Copy link
Author

wallrj's avatar @wallrj commented

Thanks for all the feedback. I've started creating some separate tickets and added links to the plan:

I'll do more later.

@twisted-trac
Copy link
Author

tomprince's avatar @tomprince set owner to @wallrj

Taking this out of review, as there isn't really anything actionable in here to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant