Skip to content
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(frontend): use absolute path for src imports #9761

Merged
merged 2 commits into from May 7, 2020

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented May 7, 2020

CATEGORY

  • Refactor
  • Build / Development Environment

SUMMARY

This changes all JS imports in specs to use absolute path, which makes the code a little bit easier to read and shall make rearranging the test files easier.

Added a jsconfig.json for VS Code so that intellisense can still work.

Also upgraded prettier to fix lint.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

Make sure CI passes and other IDEs also have no problem using JS features.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@kristw @rusackas @villebro

@@ -1,4 +1,5 @@
{
"singleQuote": true,
"trailingComma": "all"
"trailingComma": "all",
"arrowParens": "avoid"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prettier changed the default behavior for arrowParent. We revert it back to the old behavior.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be another change to space between function and parentheses.

function() {} vs function () {}

I do not have strong preference in terms of styling but perhaps we can use the original to reduce amount of changes.

This is the settings from @superset-ui

{
  "arrowParens": "avoid",
  "bracketSpacing": true,
  "jsxBracketSameLine": false,
  "printWidth": 100,
  "proseWrap": "always",
  "requirePragma": false,
  "semi": true,
  "singleQuote": true,
  "tabWidth": 2,
  "trailingComma": "all",
  "useTabs": false
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@kristw kristw May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ic. If no strong objection from others I am good with this. I'll approve but wait for others to have a chance to see the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer without the space, but if there's no way to turn it off, it won't break my heart.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just read through some of the threads on that feature, and now I'm glad I don't work on that project. Yeesh!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this then? 😄

@ktmud ktmud force-pushed the js-tests-absolute-import branch from f0a08b3 to 492d404 Compare May 7, 2020 08:28
@ktmud ktmud force-pushed the js-tests-absolute-import branch from 492d404 to 93aeede Compare May 7, 2020 19:13
@codecov-io
Copy link

codecov-io commented May 7, 2020

Codecov Report

Merging #9761 into master will decrease coverage by 0.01%.
The diff coverage is 89.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9761      +/-   ##
==========================================
- Coverage   70.81%   70.80%   -0.02%     
==========================================
  Files         586      586              
  Lines       30445    30444       -1     
  Branches     3121     3120       -1     
==========================================
- Hits        21559    21555       -4     
- Misses       8772     8775       +3     
  Partials      114      114              
Flag Coverage Δ
#cypress 53.65% <42.10%> (-0.08%) ⬇️
#javascript 59.06% <75.86%> (-0.01%) ⬇️
#python 70.93% <ø> (ø)
Impacted Files Coverage Δ
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx 65.90% <ø> (-6.82%) ⬇️
...erset-frontend/src/SqlLab/components/SqlEditor.jsx 55.12% <ø> (ø)
superset-frontend/src/explore/AdhocFilter.js 97.26% <ø> (ø)
...set-frontend/src/explore/actions/exploreActions.js 44.77% <50.00%> (ø)
.../explore/components/controls/DateFilterControl.jsx 56.36% <75.00%> (ø)
superset-frontend/src/SqlLab/actions/sqlLab.js 66.81% <85.71%> (ø)
superset-frontend/src/SqlLab/components/App.jsx 77.77% <100.00%> (ø)
...set-frontend/src/SqlLab/components/QuerySearch.jsx 57.54% <100.00%> (ø)
...rset-frontend/src/SqlLab/components/QueryTable.jsx 59.25% <100.00%> (ø)
superset-frontend/src/chart/chartAction.js 59.50% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b430ea...93aeede. Read the comment docs.

@kristw kristw merged commit 13c5b13 into apache:master May 7, 2020
@rusackas
Copy link
Member

rusackas commented May 7, 2020

Jeez... I keep spending forever looking at these things, and then when I'm done, I look up and see Merged 🤣Again, this looks great, and thanks.

@ktmud ktmud deleted the js-tests-absolute-import branch May 7, 2020 22:33
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XXL 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants