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

groupByNode[s]: implement ordering like graphite does #1774

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Apr 15, 2020

fix #1767

PRE

pre

POST

post

@Dieterbe
Copy link
Contributor Author

cc @fqiu7

@Dieterbe Dieterbe changed the title groupByNode[s]: respect order. fix #1767 groupByNode[s]: implement ordering like graphite does Apr 15, 2020
@replay
Copy link
Contributor

replay commented Apr 15, 2020

LGTM but wouldn't it make sense to also update the unit tests so they check for the correct order?

@Dieterbe
Copy link
Contributor Author

i don't like unit tests that are literally the same code as the code that is under test.

@fqiu7
Copy link
Contributor

fqiu7 commented Apr 15, 2020

LGTM. I agree the ordering should be consistent.

@replay
Copy link
Contributor

replay commented Apr 15, 2020

i don't like unit tests that are literally the same code as the code that is under test.

I agree with that, but that's not how i would implement them. At the moment testGroupByNodes() just sorts the expected and the returned series before comparing. I'd remove that sorting and then make sure that the expected list of metrics is sorted as we expect it to be.

@Dieterbe
Copy link
Contributor Author

@replay ah yes. we can simply assert that we got them in the same order as how they are specified in the tests, which we can achieve by simply removing the sorting. like this?

Copy link
Contributor

@replay replay left a comment

Choose a reason for hiding this comment

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

LGTM

@Dieterbe Dieterbe merged commit cf39498 into master Apr 15, 2020
@Dieterbe Dieterbe deleted the groupbynode-respect-order branch April 15, 2020 20:57
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.

groupByNodes() does not return series in a consistent order, Graphite does
3 participants