Skip to content

Extract TestRecord object from StreamToDict#158

Merged
jml merged 24 commits intotesting-cabal:masterfrom
jml:extract-test-record
Nov 18, 2015
Merged

Extract TestRecord object from StreamToDict#158
jml merged 24 commits intotesting-cabal:masterfrom
jml:extract-test-record

Conversation

@jml
Copy link
Copy Markdown
Member

@jml jml commented Nov 3, 2015

Many places in testtools.testresult.real refer to "a dict as generated by StreamToDict" or similar. This patch dignifies it with a real name in Python, TestRecord

The new TestRecord object encapsulates most of the information in the dict and has been extracted from StreamToDict.

I've made it inherit from pyrsistent.PClass to make it immutable. I think this is a desirable property, but not essential to the patch. We could make it mutable with attrs, or by rolling our own object like in the olden days.

Notes

  • adds pyrsistent as a dependency
  • I've left a couple of XXX comments in where feedback is particularly welcome
  • neither TestRecord nor StreamToTestRecord is exposed, we can do that here or in a separate patch
  • I was unclear on the backwards compatibility promises of testtools.testresult.real so I've tried very hard to avoid changing behaviour
  • while working on this, I really wanted to move all the "stream" stuff to a separate module, _stream. I held off because:
    • I wanted to keep the patch small
    • I don't know about backwards compatibility
  • TestRecord stores timestamps as a tuple. It occurred to me late in the piece that it would be clearer to use start_time and end_time or similar.
  • gives explicit names to "status" constants, but there's still duplication here, and ideally I'd like to have something a bit more structured (maybe w/ https://github.com/twisted/constantly/)
  • extracted base class StreamToTestRecord from StreamToDict, and then made StreamToDict use (rather than inherit from) StreamToTestRecord to do its thing
  • no test changes because I've tried to do strict refactoring

Context

I'm looking at re-using some code from testtools.testresult. This patch is in the spirit of "building understanding into existing code".

Review on Reviewable

@jml jml force-pushed the extract-test-record branch from 8598bd6 to bc8b162 Compare November 9, 2015 13:28
@jml
Copy link
Copy Markdown
Member Author

jml commented Nov 9, 2015

This is now ready for review.

While they are probably OK to be exposed, let's keep this patch minimal
& not news-worthy
@jml
Copy link
Copy Markdown
Member Author

jml commented Nov 12, 2015

Occurred to me that I didn't want to expose these yet. For now, it should just be an internal refactoring.

jml added a commit that referenced this pull request Nov 18, 2015
Extract TestRecord object from StreamToDict
@jml jml merged commit 0fb16c6 into testing-cabal:master Nov 18, 2015
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.

1 participant