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

Fix clippy error on wasmtest #12844

Merged
merged 1 commit into from
Oct 10, 2024
Merged

Fix clippy error on wasmtest #12844

merged 1 commit into from
Oct 10, 2024

Conversation

jonahgao
Copy link
Member

@jonahgao jonahgao commented Oct 10, 2024

Which issue does this PR close?

Closes #12842.

Rationale for this change

The new release of wasm-bindgen-test causes clippy to throw errors.
PR fixed these errors and also verified locally that these tests can still run on Chrome.

running 2 tests

test datafusion_wasmtest::test::basic_execute ... ok
test datafusion_wasmtest::test::datafusion_test ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 filtered out

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

No

fn datafusion_test() {
basic_exprs();
basic_parse();
}

#[wasm_bindgen_test]
#[wasm_bindgen_test(unsupported = tokio::test)]
Copy link
Member Author

Choose a reason for hiding this comment

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

According to the new feature of wasm_bindgen_test, run this test with tokio::test under non-wasm targets.

@@ -87,13 +87,14 @@ mod test {

wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser);

#[wasm_bindgen_test]
#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
#[cfg_attr(not(target_arch = "wasm32"), allow(dead_code))]
Copy link
Member Author

Choose a reason for hiding this comment

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

This test can't run on non-wasm targets, so add allow(dead_code).

thread 'test::datafusion_test' panicked at datafusion/wasmtest/src/lib.rs:41:1:
cannot call wasm-bindgen imported functions on non-wasm targets

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @jonahgao

@alamb alamb merged commit 8945e7a into apache:main Oct 10, 2024
27 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 10, 2024

Merging to get CI green

@jonahgao jonahgao deleted the fix_clippy branch October 10, 2024 12:18
alamb pushed a commit to alamb/datafusion that referenced this pull request Oct 16, 2024
alamb added a commit that referenced this pull request Oct 16, 2024
* Update to arrow 53.1.0 on branch-42

* Allow deprecated functions

* Fix clippy error on wasmtest (#12844)

* Update for different file sizes

---------

Co-authored-by: Jonah Gao <jonahgao@msn.com>
notfilippo pushed a commit to notfilippo/datafusion that referenced this pull request Oct 21, 2024
alamb pushed a commit that referenced this pull request Oct 21, 2024
* Backport native and logical types from #12853

* Fix clippy error on wasmtest (#12844)

---------

Co-authored-by: Jonah Gao <jonahgao@msn.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: Clippy error on datafusion-wasmtest
2 participants