Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#5989 enhancement closed fixed (fixed)

Standardize twisted names examples to use task.react

Reported by: Richard Wall Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: Richard Wall, Thijs Triemstra Branch: branches/examples-react-5989-3
branch-diff, diff-cov, branch-cov, buildbot
Author: rwall

Description

  • Use task.react: #3270 introduced a new function task.react which runs a single function and shuts down the reactor reliably.
  • Tidy output: The results of example scripts should be neatly printed to stdout and certain expected errors should be printed to stderr.
  • Log uncaught exceptions: react takes care of logging any uncaught exceptions which in many examples are silently discarded (#4008)
  • Standard usage docs: Also add the standard command line help and usage docs to the examples
  • Optional: Non zero exit code on errors: maybe postpone this until #718 is resolved.
  • Optional: Add unit tests for the examples: See #84 for ideas about how to do this.

See also:

Attachments (4)

names-examples-5596-1.patch (11.5 KB) - added by Richard Wall 4 years ago.
Use task.react in all names examples, standardise the script output and add some tests
examples-react-5989-2.patch (11.5 KB) - added by Richard Wall 4 years ago.
An updated patch without the tests for executability
examples-react-5989-3.patch (4.0 KB) - added by Richard Wall 4 years ago.
Update patch against source:branches/examples-react-5989
examples-react-5989-4.patch (8.8 KB) - added by Richard Wall 4 years ago.
Now with twisted.usage and other responses to the tom.prince revew.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 5 years ago by Richard Wall

comment:2 Changed 5 years ago by Richard Wall

Cc: Richard Wall added

Changed 4 years ago by Richard Wall

Attachment: names-examples-5596-1.patch added

Use task.react in all names examples, standardise the script output and add some tests

comment:3 Changed 4 years ago by Richard Wall

Author: rwall
Keywords: review added

comment:4 Changed 4 years ago by Thijs Triemstra

Author: rwallthijs, rwall
Branch: branches/examples-react-5989

(In [36994]) Branching to 'examples-react-5989'

comment:5 Changed 4 years ago by Thijs Triemstra

(In [36995]) apply names-examples-5596-1.patch. refs #5989

comment:6 Changed 4 years ago by Thijs Triemstra

Author: thijs, rwallrwall
Cc: Thijs Triemstra added

Forced a build.

comment:7 Changed 4 years ago by Richard Wall

Keywords: review removed
Owner: set to Richard Wall
Status: newassigned

Thanks thjs

I can see that my new tests are badly broken.

I'll remove the new example script permissions tests and upload another patch.

Changed 4 years ago by Richard Wall

Attachment: examples-react-5989-2.patch added

An updated patch without the tests for executability

comment:8 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted
Status: assignednew

In attachment:examples-react-5989-2.patch

  • I removed the executable mode tests - which were failing on buildbot and which were never going to work reliably on an svn checkout or in windows.
  • Fixed some pyflakes and pep8 warnings in the test module
  • renamed the test_helpUsage test.

comment:9 Changed 4 years ago by Richard Wall

Oh, and that patch is against trunk.

comment:10 Changed 4 years ago by Thijs Triemstra

Can you make a patch against the branch instead?

Changed 4 years ago by Richard Wall

Attachment: examples-react-5989-3.patch added

Update patch against source:branches/examples-react-5989

comment:11 in reply to:  10 Changed 4 years ago by Richard Wall

Replying to thijs:

Can you make a patch against the branch instead?

Sorry about that. I was in a rush and couldn't get bzr to behave.

See updated patch attachment:examples-react-5989-3.patch

-RichardW.

comment:12 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall
  1. I get buildbot failures.
  2. I also get (different) errors doing `trial -u twisted.names.test.test_examples'
  3. The examples should use twisted.python.usage, instead of a using custom argument checking.
  4. It would be nice if there were some tests that actually exercise the code in the examples. As it is, twisted.names.client, twisted.names.error and twisted.names.dns could be emptied or otherwise completely changed and the tests would all still pass. (I guess this is probably beyond the scope of this ticket)

comment:13 in reply to:  12 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted

Replying to tom.prince:

  1. I get [https://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/examples-react-5989&num_builds=10 buildbot] failures.

I think you must have started the build before applying my last patch.

The test failures are in the test_executable method, which I removed in an earlier patch...

eg twisted.names.test.test_examples.GetHostByNameTests.test_executable

...should no longer be in the branch.

  1. I also get (different) errors doing `trial -u twisted.names.test.test_examples'

Fixed. That was a problem with the ExampleTestCase class that I imported from Divmod. There was an indentation problem and it was reusing a variable name as a list and then as a FilePath.

  1. The examples should use twisted.python.usage, instead of a using custom argument checking.

Since none of the examples actually have an options (only positional args) I was trying to keep it simple. But I guess its nice to demo the twisted usage class. I updated the examples and as a result I also had to update the various docs for consistency with the twisted.usage output format.

  1. It would be nice if there were some tests that actually exercise the code in the examples. As it is, twisted.names.client, twisted.names.error and twisted.names.dns could be emptied or otherwise completely changed and the tests would all still pass. (I guess this is probably beyond the scope of this ticket)

I haven't attempted this, but there is ticket #84 for better testing of example scripts. That's where I read about the Divmod example testing class.

See latest patch attachment:examples-react-5989-4.patch which is against the branch.

Changed 4 years ago by Richard Wall

Attachment: examples-react-5989-4.patch added

Now with twisted.usage and other responses to the tom.prince revew.

comment:14 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Richard Wall

A couple of points:

  • twistedchecker error:
    ************* Module twisted.names.test.test_examples
    W9013:139,0: Expected 3 blank lines, found 2
    
  • It doesn't appear to be documented, but synopsis should be a single line. I think synopsis should be the first line of the current docstring, and the rest should be the longdesc. Also, if you move the option definition up to the top, it would be reasonable to shorten the docstring to just a single line summarizing what the program does.

(Note: I don't think there are any existing guidelines on how to write examples, so I'm trying to create them in my reviews. Feel free to disregard this, or make alternate suggestions.)

  • I'm aware of #84, but the scope of that is too big to handle all at once. It wants to be done for a single example, first. I'd suggest one of these, except they depend on the environment to work properly.

Please commit after addressing the first two points.

comment:15 Changed 4 years ago by Richard Wall

Branch: branches/examples-react-5989branches/examples-react-5989-2

(In [37206]) Branching to 'examples-react-5989-2'

comment:16 in reply to:  14 Changed 4 years ago by Richard Wall

Replying to tom.prince:

A couple of points:

  • twistedchecker error:
    ************* Module twisted.names.test.test_examples
    W9013:139,0: Expected 3 blank lines, found 2
    

Fixed.

  • It doesn't appear to be documented, but synopsis should be a
  • single line. I think synopsis should be the first line of the
  • current docstring, and the rest should be the longdesc. Also, if
  • you move the option definition up to the top, it would be
  • reasonable to shorten the docstring to just a single line
  • summarizing what the program does.

Done. Well I left the longdesc in the docstring, but that gets used automatically by usage.options

(Note: I don't think there are any existing guidelines on how to write examples, so I'm trying to create them in my reviews. Feel free to disregard this, or make alternate suggestions.)

I agree. I think it looks better. Also found a that you can do str(options) to get exactly the same output as from --help. So I'm using that when there are option validation errors.

  • I'm aware of #84, but the scope of that is too big to handle
  • all at once. It wants to be done for a single example,
  • first. I'd suggest one of these, except they depend on the
  • environment to work properly.

Yep.

Please commit after addressing the first two points.

Also

  • added some test module references
  • fixed some whitespace issues.
  • added a news file.
  • merged forward and rebuilt

comment:17 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: newclosed

(In [37209]) Merge examples-react-5989-2: Use task.react in twisted.names examples

Author: rwall Reviewer: thijs, tom.prince Fixes: #5989

  • Updated all twisted.names example scripts to use task.react, which shuts down the reactor reliably and logs uncaught exceptions.
  • Tidied up the output of the twisted.names example scripts. Results of all scripts are now neatly printed to stdout and certain expected errors are printed to stderr.
  • Standardized the example script --help output
  • Examples now return non-zero exit code on certain errors
  • Added some new unit tests for the twisted.names examples - testing for consistent usage and error messages and for a standard shebang line.

comment:18 Changed 4 years ago by Richard Wall

Resolution: fixed
Status: closedreopened

(In [37593]) Revert r37209: ExampleTestBase monkeypatches sys.stdout but doesn't replace it afterwards

11:55 < glyph> The example tests replace sys.stdout and don't replace it, which seems bad.
11:55 < glyph> twisted.names.test.test_examples.ExampleTestBase.setUp specifically
11:55 < exarkun> That _does_ maybe seem like it could cause a problem or two.
11:55 < glyph> Then, a logging test (just one!) fails
12:08 < rwall> glyph: That's my fault. I'll raise a new ticket and fix it.

Reopens: #5989

comment:19 Changed 4 years ago by Richard Wall

Branch: branches/examples-react-5989-2branches/examples-react-5989-3

(In [37625]) Branching to 'examples-react-5989-3'

comment:20 Changed 4 years ago by Richard Wall

Keywords: review added
Owner: Richard Wall deleted
Status: reopenednew

Ready for another review in log:branches/examples-react-5989-3

  • Now using TestCase.patch instead of manually monkey patching sys.stdout and sys.stderr - which ensures that sys is returned to its previous state after tests.
  • Build results

-RichardW.

comment:21 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [37878]) Merge examples-react-5989-3: Use task.react in twisted.names examples

Author: rwall Reviewer: thijs, tom.prince Fixes: #5989

  • Updated all twisted.names example scripts to use task.react, which shuts down the reactor reliably and logs uncaught exceptions.
  • Tidied up the output of the twisted.names example scripts. Results of all scripts are now neatly printed to stdout and certain expected errors are printed to stderr.
  • Standardized the example script --help output
  • Examples now return non-zero exit code on certain errors
  • Added some new unit tests for the twisted.names examples - testing for consistent usage and error messages and for a standard shebang line.

comment:22 Changed 4 years ago by Tom Prince

Keywords: review removed

Hmm. I guess I should have reviewed this, and not merged it. In any case, the change looks correct, and the test are now passing on the machine where they were failing:

<cyli> tomprince: blargh, I forgot to hit enter.  sorry for the delay just checked it out, bin/trial twisted passes.
<cyli> tomprince: python3 tests also pass on my machine for that branch
Note: See TracTickets for help on using tickets.