Skip to content
This repository was archived by the owner on Mar 4, 2022. It is now read-only.

Conversation

@jjasonclark
Copy link
Contributor

Only create enough random metric data required by tests
Combine uniform timed metric and non-uniform time metric tests
Remove .js from require
Extract method on mock data creation

Fixes issue #80

Only create enough random metric data required by tests
Combine uniform timed metric and non-uniform time metric tests
Remove .js from require
Extract method on mock data creation
Copy link
Contributor

@mscottx88 mscottx88 left a comment

Choose a reason for hiding this comment

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

HI @jjasonclark I'm not sure that we should really be concerned about the speed of a test case. It seems like a great deal of the actual logic testing is being removed in this PR. I'm sure that this module is still 100% code covered, but with only a couple metric data points, it doesn't seem like the depth and breadth of all the possible scenarios that needed to be considered in the construction of the code is being adequately tested in this set of changes. Unless there were some incompatible changes to the metrics provider itself that prompted these sets of changes, my recommendation is to leave it as-is. Let me know if you disagree.

@jjasonclark
Copy link
Contributor Author

Agreed, this test change isn’t really about speed. The name is a bit misleading. I’m really focused on the minimum amount of mock data needed to test the features.

I think you might be right about these changes losing a bit from the original. To get specific, do you mean the changes to the _onMetric uniform data and non-uniform data test?

I assume so. The fill method changes do the same as the old method just in a more lazy way. The mock data is created on demand instead of up front. And the tests other than _onMetric all used this interval based filling.

This leaves the _onMetric tests. I really think this was testing 3 things. Saving of all metric data, calculating averages for uniform created data, and averages for non-uniform created data.

If nothing else, I think we should split off the test for saving metric data into its own test. I’ll update the PR to include this change.

Could you expand upon the remaining 2 tests, uniform and non-uniform data? What qualities should we be targeting for these tests?

@mscottx88
Copy link
Contributor

mscottx88 commented Dec 14, 2017

Thanks Jason. As long as we're able to prove that the program is able to deal with erratic, even missing data points and produce the correct average, that'll be good.

@jjasonclark
Copy link
Contributor Author

How about a test for checking that it fills in the missing averages with 0 values when it gets a late arrival?

Would be

  1. add single data value. (this is to trigger average creation on our next call)
  2. Add single data value to 2nd time bucket
  3. Verify first average was created
  4. Add single data value to 4th time bucket
  5. Verify 2nd average was created and blank
  6. Verify 3rd average was created and it used the time value from the 4th bucket

@jjasonclark
Copy link
Contributor Author

What do you think of the new test cases @mscottx88 ?

@mscottx88
Copy link
Contributor

These look awesome! Thanks for putting those in!

@jjasonclark
Copy link
Contributor Author

If this is acceptable, could you approve the PR? I'll merge it and update the other pending PRs with this change.

Copy link
Contributor

@mscottx88 mscottx88 left a comment

Choose a reason for hiding this comment

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

Your ability to jump into these complex algorithms is impressive. Nice work!

@jjasonclark jjasonclark merged commit 02ec9b4 into FormidableLabs:master Dec 21, 2017
@jjasonclark jjasonclark deleted the issue_80 branch December 21, 2017 23:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants