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

rf: ZENKO-285 Single test for range calculation #274

Merged
merged 1 commit into from
May 31, 2018

Conversation

bennettbuchanan
Copy link

No description provided.

Copy link
Contributor

@jonathan-gramain jonathan-gramain left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@rahulreddy rahulreddy left a comment

Choose a reason for hiding this comment

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

Isn't the earlier setup the right way of writing tests? With this approach, you are testing multiple cases in one test. This would go against our a test should test only one thing approach.

@bennettbuchanan
Copy link
Author

I was viewing this unit as a test of the range calculation (while across multiple inputs, it's testing the same calculation). The primary motivation, though, is to improve the test suite usability—i.e. encapsulating the output of ~9000 test statements into a single one.

@jonathan-gramain
Copy link
Contributor

In this particular case I think it's fine because it's essentially testing one thing (with 10000 different inputs though), but having 10000 separate tests is overkill and clutters the output making it hard to get the result from other tests, so I suggested this change to Bennett.

@jonathan-gramain
Copy link
Contributor

I've also observed the complete unit tests run to take more than 30 seconds to complete and show a status after the last test has been executed, I'm strongly suspecting this change will significantly reduce this time.

rahulreddy
rahulreddy previously approved these changes May 29, 2018
@jonathan-gramain jonathan-gramain changed the base branch from z/1.0 to development/8.0 May 31, 2018 18:10
@ironman-machine ironman-machine dismissed stale reviews from jonathan-gramain and rahulreddy May 31, 2018 18:10

Do it again human slave!:point_right: :runner: (Oh and the pull request has been updated, by the way.)

@ironman-machine
Copy link
Contributor

PR has been updated. Reviewers, please be cautious.

@jonathan-gramain jonathan-gramain merged commit 7fc7a04 into development/8.0 May 31, 2018
@jonathan-gramain jonathan-gramain deleted the rf/ZENKO-285/singleUnitTest branch May 31, 2018 18:27
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.

None yet

4 participants