fix: Additional ResultSet tests #14741

AAfghahi · 2021-05-20T18:31:31Z

SUMMARY

This is addressing the fix found in: #14719

This adds testing to the ResultSet spec file that checks to make sure that either queryLimit or query.results.limit is present. Without either of these ResultSet throws an error which crashes SqlLab.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

This adds two new tests to ResultSet, checking to make sure that it renders correctly if either of these are present. I also added limit to the query fixture.

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2021-05-20T23:08:11Z

Codecov Report

Merging #14741 (1f48a0e) into master (a9d888a) will decrease coverage by 0.14%.
The diff coverage is 75.73%.

❗ Current head 1f48a0e differs from pull request most recent head 526d51a. Consider uploading reports for the commit 526d51a to get more accurate results

@@            Coverage Diff             @@
##           master   #14741      +/-   ##
==========================================
- Coverage   77.54%   77.39%   -0.15%     
==========================================
  Files         959      962       +3     
  Lines       48696    49150     +454     
  Branches     5743     6183     +440     
==========================================
+ Hits        37762    38042     +280     
- Misses      10733    10905     +172     
- Partials      201      203       +2

Flag	Coverage Δ
hive	`?`
javascript	`72.45% <67.56%> (-0.08%)`	⬇️
presto	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (ø)`
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
...uperset-frontend/src/components/Form/FormLabel.tsx	`64.70% <0.00%> (-4.05%)`	⬇️
...components/DashboardBuilder/DashboardContainer.tsx	`100.00% <ø> (ø)`
...ashboard/components/gridComponents/ChartHolder.jsx	`77.77% <ø> (+0.94%)`	⬆️
...nents/nativeFilters/FilterBar/FilterSets/index.tsx	`77.00% <ø> (ø)`
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`27.77% <ø> (-61.12%)`	⬇️
...odal/FiltersConfigForm/FilterScope/FilterScope.tsx	`86.20% <ø> (ø)`
...tersConfigModal/Footer/CancelConfirmationAlert.tsx	`100.00% <ø> (ø)`
...nd/src/dashboard/components/nativeFilters/utils.ts	`80.48% <ø> (ø)`
... and 124 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 a9d888a...526d51a. Read the comment docs.

superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

AAfghahi · 2021-05-21T17:36:56Z

@michael-s-molina thank you for the suggestions! I committed them all.

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

geido

LGTM! Just a nit

geido · 2021-05-24T13:19:11Z

superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

-
+  },
+};
+describe('ResultSet', () => {


Let's go straight to the test without nesting in describe. That's the approach we have been trying to use lately

geido · 2021-05-24T13:19:48Z

superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

@@ -182,3 +182,16 @@ describe('ResultSet', () => {
    });
  });
 });
+
+describe('RTL ResultSet tests', () => {


Same as my other comment concerning the usage of describe

AAfghahi · 2021-05-24T17:20:21Z

@geido like this?

hughhhh

🛳

eschutho

Thanks @AAfghahi, looks great!

* master: (163 commits) fix(native-filters): Manage default value of filters by superset (apache#14785) fix: Additional ResultSet tests (apache#14741) chore: added BasicParametersMixin to Redshift (apache#14752) fix: make dataset list sort case insensitive (apache#14528) fix: use encodeURIComponent when getting table metadata (apache#14790) fix: ensure engine is outside parameters (apache#14787) database modal should close on connect with tab layout (apache#14771) feat(native-filters): add search all filter options (apache#14710) fix: extra query in Dashboard when native filter enabled (apache#14770) chore: Improves the native filters UI/UX - iteration 2 (apache#14753) fix(native filters): Fix explore state (apache#14779) fix(explore): DndColumnSelect not handling controls with "multi: false" (apache#14737) feat: Create BigQuery Parameters for DatabaseModal (apache#14721) feat: enable user impersonation in GSheets (apache#14767) fix: add DB should not say it's Postgres (apache#14766) Revert "fix(dashboard): multiple query trigger when native filter enabled (apache#14734)" (apache#14762) feat: save database with new dynamic form (apache#14583) fix: save non-parameter DBs (apache#14759) chore: Removes ColorSchemeControl.less (apache#14199) fix(explore): Icons width (apache#14717) ...

sujeetpillai · 2021-06-02T10:25:12Z

superset-frontend/src/SqlLab/components/ResultSet.tsx

    const limit = queryLimit || results.query.limit;
+    const isAdmin = !!this.props.user?.roles.Admin;


I'm getting an error in SQLLab here if the this.props.user.roles is undefined. Changing this code to the below fixes the issue. If roles is undefined assume it's not admin. I'm not sure why roles wouldn't be populated though. I was logged in as an Administrator.

const isAdmin = !!this.props.user?.roles?.Admin;

Since this is only used to change the display message, I don't think the above fix will break any functionality.

I think that makes sense to me, @eschutho what do you think?

That looks good. @suddjian did make a fix whereby localstorage was overwriting user roles, but I think this works as a guard against any other issues that may be creeping up. At some point maybe we can take another look into why or when a user wouldn't have any roles.

AAfghahi · 2021-06-02T18:29:49Z

@sujeetpillai I did this fix here: #14953

sujeetpillai · 2021-06-04T05:27:52Z

Thanks. This looks good.

* fixing tests * added testing * Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * removed decribe Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

fixing tests

5800efc

pull-request-size bot added the size/L label May 20, 2021

AAfghahi force-pushed the ch15778_returnMessagesTest branch from dab4022 to a790144 Compare May 20, 2021 18:32

AAfghahi changed the title ~~fix: Ch15778 return messages test~~ fix: Additional ResultSet tests May 20, 2021

added testing

baf9711

AAfghahi force-pushed the ch15778_returnMessagesTest branch from a790144 to baf9711 Compare May 20, 2021 21:04

michael-s-molina reviewed May 21, 2021

View reviewed changes

AAfghahi and others added 4 commits May 21, 2021 13:36

Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

663d3f6

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

85320b7

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

753c6b4

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

5da9ecf

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/spec/javascripts/sqllab/ResultSet_spec.jsx

1497624

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

AAfghahi force-pushed the ch15778_returnMessagesTest branch from b1242d4 to 1497624 Compare May 21, 2021 17:38

michael-s-molina approved these changes May 22, 2021

View reviewed changes

geido approved these changes May 24, 2021

View reviewed changes

removed decribe

526d51a

hughhhh self-requested a review May 24, 2021 21:41

hughhhh approved these changes May 24, 2021

View reviewed changes

hughhhh merged commit 252c64b into apache:master May 24, 2021

eschutho reviewed May 25, 2021

View reviewed changes

sujeetpillai reviewed Jun 2, 2021

View reviewed changes

eschutho deleted the ch15778_returnMessagesTest branch June 2, 2021 18:26

AAfghahi mentioned this pull request Jun 2, 2021

fix: additional safeguard for ResultSet #14953

Merged

8 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Additional ResultSet tests #14741

fix: Additional ResultSet tests #14741

AAfghahi commented May 20, 2021

codecov bot commented May 20, 2021 •

edited

Loading

AAfghahi commented May 21, 2021

geido left a comment

geido May 24, 2021

geido May 24, 2021

AAfghahi commented May 24, 2021

hughhhh left a comment

eschutho left a comment

sujeetpillai Jun 2, 2021 •

edited

Loading

AAfghahi Jun 2, 2021

eschutho Jun 2, 2021

AAfghahi commented Jun 2, 2021

sujeetpillai commented Jun 4, 2021

		const limit = queryLimit \|\| results.query.limit;
		const isAdmin = !!this.props.user?.roles.Admin;

fix: Additional ResultSet tests #14741

fix: Additional ResultSet tests #14741

Conversation

AAfghahi commented May 20, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented May 20, 2021 • edited Loading

Codecov Report

AAfghahi commented May 21, 2021

geido left a comment

Choose a reason for hiding this comment

geido May 24, 2021

Choose a reason for hiding this comment

geido May 24, 2021

Choose a reason for hiding this comment

AAfghahi commented May 24, 2021

hughhhh left a comment

Choose a reason for hiding this comment

eschutho left a comment

Choose a reason for hiding this comment

sujeetpillai Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

AAfghahi Jun 2, 2021

Choose a reason for hiding this comment

eschutho Jun 2, 2021

Choose a reason for hiding this comment

AAfghahi commented Jun 2, 2021

sujeetpillai commented Jun 4, 2021

codecov bot commented May 20, 2021 •

edited

Loading

sujeetpillai Jun 2, 2021 •

edited

Loading