Add test for checking ordering of customers #136
Conversation
|
After this is merged I use a separate PR for the Changelog? Or are those for releases only and not unreleased? The Notion page for the versioning policy seems to be blocked for me today. For some reason, I do not have access to view the page. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
==========================================
+ Coverage 51.17% 52.26% +1.08%
==========================================
Files 65 65
Lines 10708 10881 +173
==========================================
+ Hits 5480 5687 +207
+ Misses 5228 5194 -34 ☔ View full report in Codecov by Sentry. |
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
053cf6e to
2fa8f07
Compare
|
Writing down the changes made in each PR should be done within each PR. So please document the change in CHANGELOG.md in this PR. |
|
Could you also create an issue for this PR? That issue has to contain why we need to insert node.reverse() in |
src/graphql/outlier.rs
Outdated
| let to = earliest_outlier_key(&after)?; | ||
| iter_to_nodes_with_outlier(iter, &to, cmp::Ordering::is_ge, last, filter, true) | ||
| } else { | ||
| iter_to_nodes_with_outlier(iter, &[], always_true, last, filter, true) | ||
| }?; | ||
| nodes.reverse(); |
There was a problem hiding this comment.
It would also be beneficial if we have a test on outliers API, that looks very similar to check_customer_ordering, so that we are sure that edges in the outliers graphql API response are in ascending order after this change.
There was a problem hiding this comment.
Sounds good! I'll write the outlier test and ping you
There was a problem hiding this comment.
From what I have tested the version without this change, I've got proper results like the attached screenshots. So did I from other APIs like clusters, eventList, and models. This means adding reverse is not necessary at all.
There was a problem hiding this comment.
Besides, the part where you test customerList should include other cases which use after and/or last just like the above screenshots do.
There was a problem hiding this comment.
If other APIs rather than customerList need to be tested because they differ regarding how they use the changed code, I think you should test those as well.
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
0eb1604 to
903a5cd
Compare
|
@marioCluml @sophie-cluml , as I put a comment in the UI repo, |
|
Since the change in |
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
add26b5 to
153b5a1
Compare
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
153b5a1 to
f44679d
Compare
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
721eccc to
b1661a1
Compare
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
084e0d8 to
ac945f7
Compare
Remove outlier changes for different PR Fix outlier to have correct pagination Remove outlier reverse changes Remove outlier changes in CHANGELOG Add more cases for customers Change change log Fix unreleased section to have no date
marioCluml
force-pushed
the
mario/customer-unit-test
branch
from
ac945f7 to
7900e95
Compare
Closes: #140 ; Closes: #139
OutlierandRankedOutlierordering does not need to be changed. However, some APIs includingCustomersdoes not have unit tests to check the ordering of edges and some may useloaddifferently. In doing so, unit tests are required to confirm. This PR is test case forCustomersfirst and the two APIs that need unit tests (NodeStatusListandAccountList) will come in a different PR.