-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: Adds tests to dnd controls #13650
test: Adds tests to dnd controls #13650
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13650 +/- ##
==========================================
+ Coverage 77.83% 78.14% +0.31%
==========================================
Files 934 934
Lines 47320 47320
Branches 5913 5929 +16
==========================================
+ Hits 36831 36980 +149
+ Misses 10346 10197 -149
Partials 143 143
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@michael-s-molina Thanks for adding test to dnd, this feature is completed for phase 1 poc and is still evolving rapidly. there's no harm adding test though. 🙏 @kgabryje please review with a grey check mark, hopefully it turns green in days. 😉 |
superset-frontend/src/explore/components/controls/DndColumnSelectControl/OptionWrapper.test.tsx
Show resolved
Hide resolved
1 nit, other than that LGTM! |
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!
<Option | ||
index={index} | ||
clickClose={clickClose} | ||
onShiftOptions={onShiftOptions} |
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.
Not sure exactly sure what the implications/risks may be of removing this, if 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.
I checked when I removed it 😉. Nice observation though!
export const DragContainer = styled.div`
margin-bottom: ${({ theme }) => theme.gridUnit}px;
:last-child {
margin-bottom: 0;
}
`;
9397305
to
d2526b7
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!
* master: (26 commits) chore: bump to new superset-ui version (#13932) fix: do not run containers as root by default in Helm chart (#13917) feat(explore): adhoc column formatting for Table chart (#13758) fix(sqla-query): order by aggregations in Presto and Hive (#13739) feat(alert/report): add ALERTS_ATTACH_REPORTS feature flags + feature (#13894) test: Fixes PropertiesModal_spec (#13548) fix: Pin Prophet dependency after breaking changes (#13852) test: Adds tests to dnd controls (#13650) test: Adds tests to the AnnotationLayer component (#13748) test: Refactor and enhance tests for the Explore DatasourcePanel Component (#13799) Add tests (#13778) test: DisplayQueryButton (#13750) Fixing condition around left margin for dashboard layout. Fixes #13863 (#13905) Revert "fix: select table overlay (#13694)" (#13901) test: Adds tests to the OptionControls component (#13729) test: DatasourceControl (#13605) tests for function handleScroll (#13896) test: Adds tests to the CustomFrame component (#13675) test: Adds tests to the AdvancedFrame component (#13664) test: DataTableControl (#13668) ...
SUMMARY
Adds tests to dnd controls.
@kgabryje We are auditing the codebase of Explore searching for components that don’t have sufficient test coverage (at least 80%). We are using Codecov reports to find the components. From what I understood this feature is still in POC mode, right? Anyway, I added some test files here to help with the test coverage and serve as a base for more complete tests. Can you please review it?
PS:
DndMetricSelect.test.tsx
has only one test. After we add typescript support toDndMetricSelect
props
we should complement this.TEST PLAN
1 - Execute all drag and drop tests.
2 - All tests should pass.
ADDITIONAL INFORMATION