Skip to content

Add two functions that assist in testing a TypedPipe#1478

Merged
johnynek merged 4 commits intotwitter:developfrom
richwhitjr:develop
Jan 22, 2016
Merged

Add two functions that assist in testing a TypedPipe#1478
johnynek merged 4 commits intotwitter:developfrom
richwhitjr:develop

Conversation

@richwhitjr
Copy link
Copy Markdown
Contributor

Many times when testing a scalding job you just want to test a function that takes a TypedPipe and returns another TypedPipe. This can be difficult using JobTest. TypedPipeChecker makes use of Executions to pull the given TypedPipe into memory so that can asserts can be run against the resulting List.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having a flavor like

def checkOutputTransform[T, U](input: List[T])(transform: TypedPipe[T] => TypedPipe[U])(assertions: List[T] => Unit) =
  assertions(checkOutputInline(transform(TypedPipe.from(input))))

Or doing TypedPipe.from we should leave to the test writer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure that actually may be nice to have.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function added

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here: make the return generic please.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we put a return type on all methods?

also, what about mre general:

def checkOutputTransform[T, U, R](input: List[T])(transform: TypedPipe[T] => TypedPipe[U])(assertions: List[U] => R): R =

then you could use methods that return other than unit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason not to call this inMemoryToList or something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this name for this function. I kind of like idea of keeping the names for the others though since it really is just a nice to have utility function for running some asserts on a typed pipe and the name indicates that it probably should only be used for unit testing.

If you think it could be used outside of tests though we can try to think of some better names.

@johnynek
Copy link
Copy Markdown
Contributor

ready to merge, just some name bikeshedding.

@isnotinvain
Copy link
Copy Markdown
Contributor

Is there any need for the checkOutputTransform part?
Why not just keep the inMemoryToList part, and then let users use it directly, eg:

val expected = List(1,2,3)
val found = inMemoryToList(myTestedMethod(TypedPipe.from(List("a", "b", "c"))))
assert(found === expected)

@richwhitjr
Copy link
Copy Markdown
Contributor Author

Mostly just a simple utility function. Not strictly needed. You could imagine have a function that checks a TypedPipe that you may want reuse on different tests so it could be a little cleaner to use the checkOutput function.

Sometimes just having the function where you are doing the testing can make the test a little more readable.

I can see the transform being useful when you have some function of TypedPipe[T] -> TypedPipe[U] and you want to test this. This is common in many of my real world tests.

johnynek added a commit that referenced this pull request Jan 22, 2016
Add two functions that assist in testing a TypedPipe
@johnynek johnynek merged commit f57e0dd into twitter:develop Jan 22, 2016
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.

4 participants