-
Notifications
You must be signed in to change notification settings - Fork 40
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
test: Convert explain count tests to new explain setup #1488
test: Convert explain count tests to new explain setup #1488
Conversation
sources in one count request, and convert all the tests to the new explain setup.
Codecov Report
@@ Coverage Diff @@
## develop #1488 +/- ##
===========================================
+ Coverage 72.12% 72.18% +0.06%
===========================================
Files 185 185
Lines 18239 18239
===========================================
+ Hits 13154 13166 +12
+ Misses 4041 4035 -6
+ Partials 1044 1038 -6 |
de0be7c
to
b549e6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one question :)
{ | ||
TargetNodeName: "scanNode", // inside of root | ||
OccurancesToSkip: 0, | ||
IncludeChildNodes: true, // should be leaf of it's branch, so will have no child nodes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: I saw this on the other PR I approved but now I'm starting to scratch my head when looking at the comment. Is it normal that the field says true to include child nodes but the comment says "will have no child node"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scanNode
is a special case as it should be the last node (leaf) in all these cases. The IncludeChildNodes: true
is there to ensure that there are no nodes inside / under the scanNode
. If some child nodes were ever introduced in the future, it would ensure to fail the test (as assumptions have changed) and should deliberately handle/change the test then based on the new change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks for the explanation. I do feel like the name is a little misleading though. Out of scope here of course but maybe something we can revisit later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IncludeChildNodes
includes child nodes if they are available. I thought it covered the purpose well. Open to suggestions if you have any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I get it now. It’s the comment on hat threw me off. I thought the comment was saying that we set it to true because there will be no child nodes. Not “although” it’s set to true, there will be no child nodes.
b549e6c
to
c1e487b
Compare
…#1488) ## Relevant issue(s) - Part of sourcenetwork#953 - Resolves sourcenetwork#1486 ## Description Continue converting explain tests to the new explain setup before we can integrate the entire setup to the new action-based setup. sourcenetwork#953 Has a lot more detail on the entire plan. - This PR converts all the default explain count tests. - Splits count join tests in a separate file. - Adds some new tests that were missed before.
Relevant issue(s)
Description
Continue converting explain tests to the new explain setup before we can integrate the entire setup to the new action-based setup. #953 Has a lot more detail on the entire plan.