Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Mar 7, 2025

Which issue does this PR close?

Draft as it build on #13672

Rationale for this change

Let's keep the CI runs quick and efficient

While datafusion-cli is part of the main workspace now (and will get tested with cargo test I think it should be its own CI job because:

  1. It has a bunch of extra dependencies so not compiling it will make the main test job slightly faster
  2. After Add insta / snapshot testing to CLI & set up AWS mock #13672 it also uses a docker container and some environment variables and I would like to keep the base CI test as simple as possible

What changes are included in this PR?

Move datafuson-cli test run to its own separate CI job

Are these changes tested?

Yes, see CI jobs below

  • Example test job (showing it is not running datafusion-cli and shorter than main): TODO
  • Example datafusion-cli test job:

Timing summary
Compared to this recent main CI runs (click links)

job main this branch
tests 13m54s 13m36s
datafusion-cli tests N/A 4m

Are there any user-facing changes?

@github-actions github-actions bot added documentation Improvements or additions to documentation development-process Related to development process of DataFusion labels Mar 7, 2025
@alamb alamb force-pushed the alamb/separate_cli_ci branch from 234c8ec to 327c1e4 Compare March 11, 2025 15:09
@github-actions github-actions bot removed the documentation Improvements or additions to documentation label Mar 11, 2025
@alamb alamb marked this pull request as ready for review March 11, 2025 19:16
run: git diff --exit-code

# datafusion-cli tests
linux-test-datafusion-cli:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the point is to split the run into its own job into its own test

Copy link
Collaborator

Choose a reason for hiding this comment

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

was it intentional to keep cli tests for mac?

Copy link
Contributor Author

@alamb alamb Mar 11, 2025

Choose a reason for hiding this comment

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

It was not -- I will remove it. Good point

Update: I actually think it is good to have the mac tests still run (even if not integration tests) to provide basic coverage

Copy link
Collaborator

Choose a reason for hiding this comment

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

fair

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking especially newlne / formatting nonsense that is subtly different on mac

- name: Run tests (excluding doctests and datafusion-cli)
env:
RUST_BACKTRACE: 1
run: |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

datafusion-cli is excluded here now

I also expanded out this list of tests to make it easier to read

Copy link
Collaborator

@blaginin blaginin 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!!!

@alamb alamb requested a review from comphead March 13, 2025 20:12
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 @alamb its about time, for each platform we recompile and run tests for datafusion cli, although just having a linux is enough

@alamb
Copy link
Contributor Author

alamb commented Mar 14, 2025

lgtm thanks @alamb its about time, for each platform we recompile and run tests for datafusion cli, although just having a linux is enough

THanks -- filed a ticket to track this

@alamb alamb merged commit 9fd7e6f into apache:main Mar 14, 2025
25 checks passed
@alamb alamb deleted the alamb/separate_cli_ci branch March 14, 2025 12:41
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.

3 participants