-
Notifications
You must be signed in to change notification settings - Fork 14k
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 the AnnotationLayer component #13748
test: Adds tests to the AnnotationLayer component #13748
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13748 +/- ##
==========================================
+ Coverage 77.83% 78.00% +0.17%
==========================================
Files 934 934
Lines 47320 47320
Branches 5913 5931 +18
==========================================
+ Hits 36831 36912 +81
+ Misses 10346 10264 -82
- Partials 143 144 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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!
/testenv up |
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.
Minor nit, other than that LGTM!
import SelectControl from './SelectControl'; | ||
import TextControl from './TextControl'; | ||
import CheckboxControl from './CheckboxControl'; | ||
|
||
import SelectControl from 'src/explore/components/controls/SelectControl'; | ||
import TextControl from 'src/explore/components/controls/TextControl'; | ||
import CheckboxControl from 'src/explore/components/controls/CheckboxControl'; |
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 personally prefer relative imports if they're in the same directory, but I don't have strong opinions on this. But it would be nice if we could enforce this with with some linting rules, e.g. requiring relative imports if in the same or parent directory, and absolute if elsewhere.
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.
@villebro I agree with you. I'm using relative imports if they're in the same directory and full paths otherwise. Since in this case, they are not in the same directory anymore, I used full paths. Totally agree with the Lint rules.
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.
My apologies, I didn't notice that this file moved 🤦 My "internal" linter is ok with relative imports from parent directories, but absolute is also just fine! But it would be great if the linter enforced fully deterministic imports (will try to poke around to see if this is possible/makes sense)
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.
No problem at all! 😄
But it would be great if the linter enforced fully deterministic imports (will try to poke around to see if this is possible/makes sense)
This would be awesome!
@villebro @rusackas I think that after SIP-61 we'll have a more predictable structure, and that can allow us to use Webpack module aliases for our main folders and help with full imports.
@junlincc Ephemeral environment spinning up at http://52.41.255.186:8080. Credentials are |
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! Thanks - just needs a slight rebase.
36152cd
to
60722af
Compare
Rebased 😉 |
Ephemeral environment shutdown and build artifacts deleted. |
* 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 the
AnnotationLayer
component.TEST PLAN
1 - Execute
AnnotationLayer
tests2 - All tests should pass
@rusackas @junlincc
ADDITIONAL INFORMATION