Skip to content

Conversation

@XiangpengHao
Copy link
Contributor

@XiangpengHao XiangpengHao commented Apr 5, 2025

Which issue does this PR close?

Related to #13815 #15102

Rationale for this change

While digging into another issue, I realized the wasm test was never actually executed in CI, and thus we don't know whether our previous tests will pass (although they build successfully)

  1. This PR enables running wasm test in CI. I believe headless mode is supported as of 2025.

  2. The snapshot test doesn't seem to support wasm: Supporting wasm-bindgen-test? mitsuhiko/insta#120, so I disabled it and has to use some ugly string match

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the development-process Related to development process of DataFusion label Apr 5, 2025
@XiangpengHao
Copy link
Contributor Author

I don't fully understand why the testing drivers keep getting killed. It works on my local machine and on other CI..https://github.com/rustwasm/gloo/blob/master/.github/workflows/tests.yml#L79

Will take a closer look later today

@XiangpengHao XiangpengHao marked this pull request as draft April 5, 2025 17:33
@XiangpengHao XiangpengHao marked this pull request as ready for review April 5, 2025 21:28
@XiangpengHao
Copy link
Contributor Author

I apologize for the super long CI tuning; it has been one of the worst CI experiences...

This chrome testing thing is very difficult to setup because it will SIGKILL if the chrome version doesn't matches with the chrome driver version, making it almost impossible to debug.

I eventually switched back to the default chrome driver and default chrome, which uses the default runner image environment: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#browsers-and-drivers

I think it's not great to have this task running in a separate env setup, but it has been a huge pain to make it work; anyone feel free to improve it...

@alamb
Copy link
Contributor

alamb commented Apr 6, 2025

Thank you @XiangpengHao -- I also find CI debugging very long and painful

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 @XiangpengHao

I double chekced the action works: https://github.com/apache/datafusion/actions/runs/14285896146/job/40040931834?pr=15595

Also, cc @jonmmease who I think contributed the original tests for wasm

working-directory: ./datafusion/wasmtest
run: wasm-pack build --dev
run: |
wasm-pack test --headless --firefox
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

@qstommyshu
Copy link
Contributor

This could be very helpful for implementing CI for datafusion-wasm-bindings as well!

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @XiangpengHao

@comphead comphead merged commit 362fcdf into apache:main Apr 6, 2025
27 checks passed
@comphead
Copy link
Contributor

comphead commented Apr 6, 2025

This could be very helpful for implementing CI for datafusion-wasm-bindings as well!

@qstommyshu would you mind creating a ticket?

nirnayroy pushed a commit to nirnayroy/datafusion that referenced this pull request May 2, 2025
* actually run test in ci

* tweak ci

* try ci again

* try ci again

* try ci again

* setup debug

* update

* try again

* update ci

* install jq

* update ci

* fix ci

* update

* update

* try again

* Update .github/workflows/rust.yml

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants