-
Notifications
You must be signed in to change notification settings - Fork 22
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
refactor(react-components): unit tests for rule based coloring: core and sub components #5014
base: master
Are you sure you want to change the base?
refactor(react-components): unit tests for rule based coloring: core and sub components #5014
Conversation
…0-unit-tests-for-rule-based-coloring
…tetime in number to avoid too deeper precision in datetime milliseconds
…0-unit-tests-for-rule-based-coloring
…0-unit-tests-for-rule-based-coloring
It will be back once the v10 is in reveal
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5014 +/- ##
===========================================
- Coverage 71.10% 32.00% -39.11%
===========================================
Files 378 667 +289
Lines 38591 54383 +15792
Branches 2774 760 -2014
===========================================
- Hits 27442 17407 -10035
- Misses 11039 36753 +25714
- Partials 110 223 +113
|
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 couple of harmless comments
type: 'size', | ||
fill: '', | ||
label: 'Size' | ||
} as unknown as RuleOutput; |
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.
Is this unknown-casting necessary? Can we instead add data into the object to make it fit the type? Or are we testing something that isn't really covered by the type system (in that case, I would actually rather that we either change the type or just don't test this case at all)
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 was considering a potential case if the type is non-type defined in RuleOutput type definitions (like canvasAnnotation, email, etc. that is covered by RuleOutput type but we dont support those yet) and this test just throw anything else other than "color" type.... but maybe you're right.. maybe we dont need to cover this case as well in here. I will remove it
Type of change
Jira ticket 📘
https://cognitedata.atlassian.net/browse/
Description 📝
Unit test for the Rule Based Outputs for the functions and components in the subfolders: core and components.
Note: the MVVM refactoring will take place at the upper level components on the other ticket related to the unit test and MVVM for the other components like hooks and the entry point components like RuleBasedOutputSelector which calls the item components from this ticket and the entry Button.
How has this been tested? 🔍
Test instructions ℹ️
Checklist ☑️