-
Notifications
You must be signed in to change notification settings - Fork 44
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(i): Test top level agg. wt compound relational filter #1870
test(i): Test top level agg. wt compound relational filter #1870
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
@@ Coverage Diff @@
## develop #1870 +/- ##
===========================================
+ Coverage 70.38% 70.41% +0.02%
===========================================
Files 232 232
Lines 24180 24180
===========================================
+ Hits 17018 17024 +6
+ Misses 6001 5997 -4
+ Partials 1161 1159 -2
Flags with carried forward coverage won't be shown. Click here to find out more. see 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
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 a non-blocking name suggestion.
@@ -196,17 +196,24 @@ func init() { | |||
} | |||
} | |||
|
|||
// AssertPanicAndSkipChangeDetection asserts that the code of function actually panics, | |||
// AssertPanic asserts that the code inside the specified PanicTestFunc panics. |
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.
nitpick(non-blocking): Rename suggestion
// AssertPanic asserts that the code inside the specified PanicTestFunc panics. | |
// AssertPanicOrSkip asserts that the code inside the specified PanicTestFunc panics. |
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.
:) I do disagree here, although I think my views over time have shifted, and I might have been the one to originally ask for the longer name. Will leave as AssertPanic
unless anyone else requests the change before I rebase and merge.
3bf0f63
to
959c79e
Compare
…work#1870) ## Relevant issue(s) Resolves sourcenetwork#1868 ## Description Adds a test for aggregates with a compound relational filter. Documents sourcenetwork#1869
Relevant issue(s)
Resolves #1868
Description
Adds a test for aggregates with a compound relational filter.
Documents #1869
I believe this predates the release, and may be a fiddly fix with lots more testing required, so I'm documenting it now and moving on - maybe it will be fixed before the release goes out, but will see.