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

chore: Reenable SQLite tests which leverage foreign key constraints et al. #24605

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jul 6, 2023

SUMMARY

In #24488 I added logic to enable foreign key support for SQLite. A positive byproduct of said change is there are a slew of SQLite integration tests which were disabled due to the lack of foreign key support (and/or blind copypasta).

This PR simply reenables the previously skipped SQLite tests.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • 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

@john-bodley john-bodley changed the title chore: Renable SQLite tests which leverage foreign key constraints [WIP] chore: Renable SQLite tests which leverage foreign key constraints Jul 6, 2023
@john-bodley john-bodley changed the title [WIP] chore: Renable SQLite tests which leverage foreign key constraints chore: [WIP] Renable SQLite tests which leverage foreign key constraints Jul 6, 2023
@john-bodley john-bodley marked this pull request as ready for review July 6, 2023 17:02
@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch from a96ff8d to 910de2c Compare July 6, 2023 17:51
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9033e72) 69.14% compared to head (4651688) 69.14%.
Report is 11 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #24605   +/-   ##
=======================================
  Coverage   69.14%   69.14%           
=======================================
  Files        1946     1946           
  Lines       75989    75989           
  Branches     8479     8479           
=======================================
  Hits        52544    52544           
  Misses      21266    21266           
  Partials     2179     2179           
Flag Coverage Δ
hive 53.68% <ø> (ø)
mysql 78.05% <ø> (ø)
postgres 78.17% <ø> (ø)
presto 53.63% <ø> (ø)
python 82.86% <ø> (ø)
sqlite 77.76% <ø> (+0.93%) ⬆️
unit 55.80% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch 6 times, most recently from 5ede24b to f388c36 Compare July 12, 2023 23:30
@@ -2498,9 +2333,11 @@ def test_get_or_create_dataset_creates_table(self):
self.assertEqual(table.template_params, '{"param": 1}')

db.session.delete(table)
db.session.commit()
Copy link
Member Author

Choose a reason for hiding this comment

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

See here, specifically it mentions,

SQLite locks the database when a write is made to it, such as when an UPDATE, INSERT or DELETE is sent. When using the ORM, these get sent on flush. The database will remain locked until there is a COMMIT or ROLLBACK.

hence the need for the commit to free the lock prior to dropping the actual table.

@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch from f388c36 to 0472b0f Compare July 12, 2023 23:37
@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch 3 times, most recently from 4101492 to 9773aeb Compare January 2, 2024 20:54
@john-bodley john-bodley marked this pull request as draft January 2, 2024 20:56
@john-bodley john-bodley changed the title chore: [WIP] Renable SQLite tests which leverage foreign key constraints chore: Reenable SQLite tests which leverage foreign key constraints Jan 2, 2024
@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch 6 times, most recently from d8c3ba3 to f021d99 Compare January 3, 2024 00:19
@@ -150,7 +150,9 @@ jobs:
SUPERSET_CONFIG: tests.integration_tests.superset_test_config
REDIS_PORT: 16379
SUPERSET__SQLALCHEMY_DATABASE_URI: |
sqlite:///${{ github.workspace }}/.temp/unittest.db
Copy link
Member Author

@john-bodley john-bodley Jan 3, 2024

Choose a reason for hiding this comment

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

Weird name for a database used for integration (as opposed to unit) tests. Calling this superset.db is inline with how the metadata database is named elsewhere—including in this workflow for MySQL and PostgreSQL.

@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch 3 times, most recently from ac0bd8f to 8066ffb Compare January 3, 2024 02:55
@michael-s-molina
Copy link
Member

Not a blocker for this PR but there's a task in 4.0 to drop support for SQLite to materialize SIP-33.

@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch 7 times, most recently from 00aec58 to 5148fc8 Compare January 5, 2024 02:02
@@ -150,7 +150,9 @@ jobs:
SUPERSET_CONFIG: tests.integration_tests.superset_test_config
REDIS_PORT: 16379
SUPERSET__SQLALCHEMY_DATABASE_URI: |
sqlite:///${{ github.workspace }}/.temp/unittest.db
sqlite:///${{ github.workspace }}/.temp/superset.db?check_same_thread=true
Copy link
Member Author

@john-bodley john-bodley Jan 5, 2024

Choose a reason for hiding this comment

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

This threw me for a curveball. I was able to successfully run the tests locally but they failed remotely with a somewhat cryptic error stating a reference table being specified in this fixture didn't exist.

I speculate that this is probably due to a multithreading or connection pooling issue when running via a GitHub workflow. Ensuring a single connection per thread seems to remedy the problem. 🤞

@@ -150,7 +150,9 @@ jobs:
SUPERSET_CONFIG: tests.integration_tests.superset_test_config
REDIS_PORT: 16379
SUPERSET__SQLALCHEMY_DATABASE_URI: |
sqlite:///${{ github.workspace }}/.temp/unittest.db
sqlite:///${{ github.workspace }}/.temp/superset.db?check_same_thread=true
SUPERSET__SQLALCHEMY_EXAMPLES_URI: |
Copy link
Member Author

Choose a reason for hiding this comment

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

Per here, if undefined the metadata and example databases are the same and given that SQLite doesn't support schemas. It seems prudent to split these into two after @dpgaspar's #25003 which isolated the examples database.

@john-bodley john-bodley changed the title chore: Reenable SQLite tests which leverage foreign key constraints chore: Reenable SQLite tests which leverage foreign key constraints et al. Jan 5, 2024
@john-bodley john-bodley marked this pull request as ready for review January 5, 2024 02:28
@john-bodley john-bodley force-pushed the john-bodley--chore-reenable-sqlite-tests branch from 5148fc8 to 4651688 Compare January 5, 2024 02:29
Copy link
Member

@craig-rueda craig-rueda left a comment

Choose a reason for hiding this comment

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

LGTM.

@john-bodley john-bodley merged commit 77bd7cf into apache:master Jan 8, 2024
30 checks passed
@john-bodley john-bodley deleted the john-bodley--chore-reenable-sqlite-tests branch January 8, 2024 17:33
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 4.0.0 labels Apr 17, 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/L 🚢 4.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants