Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

update idxDelete unit test to fail if it takes too long #609

Merged
merged 1 commit into from
Apr 18, 2017

Conversation

woodsaj
Copy link
Member

@woodsaj woodsaj commented Apr 18, 2017

  • the main purpose of this is to ensure that deletes of nodes with
    a large number of children dont take too long. So the unit test
    has been updated to fail if the operation takes more then 10seconds.

- the main purpose of this is to ensure that deletes of nodes with
a large number of children dont take too long. So the unit test
has been updated to fail if the operation takes more then 10seconds.
@woodsaj woodsaj mentioned this pull request Apr 18, 2017
@woodsaj woodsaj requested a review from Dieterbe April 18, 2017 10:15
Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

@woodsaj
Copy link
Member Author

woodsaj commented Apr 18, 2017

unit tests are to validate correct behavior. If this test fails , which it will if the change is the delete function is reverted, then it means something is broken.

Benchmarks dont prevent builds from completing. But the build should definitely fail if this test fails.

@woodsaj woodsaj dismissed Dieterbe’s stale review April 18, 2017 17:46

unit tests are to validate correct behavior, which is what we are doing here.

@woodsaj woodsaj merged commit 86860d8 into master Apr 18, 2017
@woodsaj woodsaj deleted the idxDeleteTest branch April 18, 2017 17:46
@Dieterbe
Copy link
Contributor

Dieterbe commented Apr 18, 2017

that's a road without an end. following the same reasoning we can start writing unit tests that we're able to ingest x points per second, that render requests can be responded to in x time, that we can add y items to the index per second and so on. there's tens of things like that that are the same as what you're doing, but it doesn't change the fact that unit tests are not a good way to keep tabs on performance characteristics. much more useful would be visualizing benchmark results over time. since a while now we already execute benchmarks in every build, but the next step we need is comparing the results over time and alerting in case of significant regressions in any benchmark.
what you're doing is also very binary. it wouldn't detect if a delete used to take 2s but suddenly takes 8s for example.

@woodsaj
Copy link
Member Author

woodsaj commented Apr 18, 2017

you are completely missing the point. We had a bug that caused an outage for a customer. This unit test ensures that bug does not re-appear.

@Dieterbe
Copy link
Contributor

that point is not lost on me. we can solve the same problem in a better way using go's benchmark feature.

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