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

ref(ci): Do not run cargo check separately and test all features #1079

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

flub
Copy link
Contributor

@flub flub commented Jun 6, 2023

The cargo check runs do not help with any speedup, they cost >20s, do
not test anything that's not already covered by other tests. So
remove them.

Updates the cargo test invocations to:

  • Use --workspace instead of deprecated --all. We don't have a
    workspace crate yet though so could just remove it for now, but this
    is a bit more future-proof I guess.

  • Use --all-features since we have no features that are incompatible
    with each other. Let's keep it that way!

  • Select the --lib --bins and --tests targets, --all-targets would
    include the benches which we probably don't want to run as part of the
    main test run, but we don't have any yet so that is also just
    future-proofing.

The cargo check runs do not help with any speedup, they cost >20s, do
not test anything that's not already covered by other tests.  So
remove them.

Updates the cargo test invocations to:

- Use --workspace instead of deprecated --all.  We don't have a
  workspace crate yet though so could just remove it for now, but this
  is a bit more future-proof I guess.

- Use --all-features since we have no features that are incompatible
  with each other.  Let's keep it that way!

- Select the --lib --bins and --tests targets, --all-targets would
  include the benches which we probably don't want to run as part of the
  main test run, but we don't have any yet so that is also just
  future-proofing.

- name: check bench
if: matrix.rust == 'nightly'
run: cargo check --target ${{ matrix.target }} --benches
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have any benches, so i removed this.

@flub
Copy link
Contributor Author

flub commented Jun 9, 2023

@Arqu any objections?

Copy link
Collaborator

@Arqu Arqu left a comment

Choose a reason for hiding this comment

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

👍 agree

@flub flub merged commit dd6eccb into x-hp Jun 9, 2023
11 of 15 checks passed
@flub flub deleted the flub/hp-ci-no-check branch June 9, 2023 10:10
- name: tests
run: cargo test --all
run: cargo test --workspace --all-features --lib --bins --tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is removing the docs tests. There is only one though..

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 catch, thanks!
#1095

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.

None yet

3 participants