-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(tesseract): Support for rolling window without date range #9364
feat(tesseract): Support for rolling window without date range #9364
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #9364 +/- ##
=======================================
Coverage 83.72% 83.72%
=======================================
Files 229 229
Lines 82574 82614 +40
=======================================
+ Hits 69138 69172 +34
- Misses 13436 13442 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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. Left minor notes rgd tests
it('rolling count without date range', async () => { | ||
if (!getEnv('nativeSqlPlanner')) return; |
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.
Why not to mark test as skipped if !getEnv('nativeSqlPlanner')
. Because now it's a bit weird: test runs every time, but in real it does nothing for !getEnv('nativeSqlPlanner')
case. With skip - it will be clear that test is skipped.
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.
IIRC, smth like this:
it('rolling count without date range', async () => { | |
if (!getEnv('nativeSqlPlanner')) return; | |
if (getEnv('nativeSqlPlanner')) { | |
it('rolling count without date range', async () => { | |
..... | |
}); | |
} else { | |
it.skip('rolling count without date range', () => {}); | |
} |
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.
Or even better:
testNativeFn = getEnv('nativeSqlPlanner')? it : it.skip;
and then use it for all tests that should run only for native:
testNativeFn('rolling count without date range', async () => { ... });
With this approach, you can put all native tests into the same room and remove all if (!getEnv('nativeSqlPlanner')) return;
checks.
be30231
to
8538a47
Compare
Check List
Issue Reference this PR resolves
[For example #12]
Description of Changes Made (if issue reference is not provided)
[Description goes here]