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

Make upload tests faster with with-uploads-table macro #38453

Merged
merged 5 commits into from
Feb 6, 2024

Conversation

calherries
Copy link
Contributor

Uploads tests are slow, especially for redshift. The reason is mostly due to using mt/with-empty-db to clean up temporary tables created by the tests. mt/with-empty-db relatively slow because it performs a database sync on the empty database.

This PR replaces many uses of mt/with-empty-db with a new macro, with-uploads-table. The new macro doesn't create a new database and instead simply cleans up after a temporary table in the app and test database.

@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Feb 5, 2024
@calherries calherries changed the title Introduce with-upload-table macro Make upload tests faster with with-uploads-table macro Feb 5, 2024
@calherries calherries requested a review from a team February 5, 2024 22:10
Copy link

replay-io bot commented Feb 5, 2024

StatusComplete ↗︎
Commited824da
Results
⚠️ 2 Flaky
2269 Passed

(let [table-ident (#'upload/table-identifier t)]
(driver/drop-table! driver/*driver* (mt/id) table-ident)))))

(defmacro with-upload-table!
Copy link
Contributor

@qnkhuat qnkhuat Feb 6, 2024

Choose a reason for hiding this comment

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

I think we can safely make these macros private

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ed612a6

(defmacro with-uploads-allowed [& body]
`(do-with-uploads-allowed (fn [] ~@body)))

(defn- do-with-upload-table! [t f]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
(defn- do-with-upload-table! [t f]
(defn- do-with-upload-table! [table thunk]

nit: short var is hard to read IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ed612a6

(driver/drop-table! driver/*driver* (mt/id) table-ident)))))

(defmacro with-upload-table!
"Execute `body` with a table created by evaluating the `table-creator` expression. The table instance is bound to
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd write a bit more about what table-creator supported to return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, this requirement was missing from the docstring. Updated in ed612a6

Execute `body` with a table created by evaluating the expression `create-table-expr`. `create-table-expr` must evaluate
  to a toucan Table instance. The instance is bound to `table-sym` in `body`. The table is cleaned up from both the test
  and app DB after the body executes.

(with-upload-table [table (create-upload-table! ...)]
...)"
{:style/indent 1}
[[table-sym table-creator] & body]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[[table-sym table-creator] & body]
[[table-binding create-table-fn] & body]
  • table-binding: I think this is how we name these kind of pseudo binding in macro
  • create-table-fn: when I first read table-creator I thought it's an user, not a function that create a table

Copy link
Contributor

Choose a reason for hiding this comment

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

oh I just realized table-creator is not a function but a table, in that case I think we can call it upload-table since as far as the macro concerned it's evaluated as a table.

Copy link
Contributor

@crisptrutski crisptrutski Feb 6, 2024

Choose a reason for hiding this comment

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

Personally I like table-expr over table-creator or upload-table.

Copy link
Contributor Author

@calherries calherries Feb 6, 2024

Choose a reason for hiding this comment

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

Good point, table-creator is a it misleading. I went with create-table-expr to make it clear that the table needs to be created inside the expression and not before it, which is the main sharp edge when using this macro. ed612a6

Copy link
Contributor

@qnkhuat qnkhuat left a comment

Choose a reason for hiding this comment

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

have a couple of styling nits, but overall this looks good

Copy link
Contributor

@crisptrutski crisptrutski left a comment

Choose a reason for hiding this comment

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

Like Ngoc I also just think a few superficial things can be improved. Nice idea!

test/metabase/upload_test.clj Outdated Show resolved Hide resolved
(with-upload-table [table (create-upload-table! ...)]
...)"
{:style/indent 1}
[[table-sym table-creator] & body]
Copy link
Contributor

@crisptrutski crisptrutski Feb 6, 2024

Choose a reason for hiding this comment

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

Personally I like table-expr over table-creator or upload-table.

(let [csv-file-prefix (str (mt/random-name) ".csv")]
(let [model-1 (upload-example-csv! :csv-file-prefix csv-file-prefix)
;; sleep for a second to make sure the table name is different
_ (Thread/sleep 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yay! No sleep for the virtuous 😄

calherries and others added 3 commits February 6, 2024 14:48
@calherries calherries merged commit a1a9430 into fix-redshift-supports-uploads Feb 6, 2024
29 checks passed
@calherries calherries deleted the make-uploads-tests-faster branch February 6, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants