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

Use matrix setup in our workflow to test features #420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Weibye
Copy link
Collaborator

@Weibye Weibye commented Apr 10, 2023

Objective

Note: This builds from #414 so this should be merged after that one.

As taffy grows in scope and we add more layouting algorithms that each has their own feature flags, maintaining the full set of features that needed to be tested together is going to be harder to keep track of and maintain manually.

This change proposes to start using github actions' matrix setup in order to more easily grow and maintain the number of features we have.

It includes the following:

  1. One matrix for testing all default features together with each the optional features
  2. One matrix for testing no default features, but each of the layouting features together with each of the optional features.

Feedback wanted

  • Does this setup make sense? Adding an entry in an array is simpler than adding new distinct jobs for each of the features.
  • There is nothing testing all features at once. Is that something we need?

Comment on lines 48 to 63
- if: ${{ matrix.layout == 'none' && matrix.feature == 'none' }}
run: |
cargo build --no-default-features
cargo test --no-default-features
- if: ${{ matrix.layout != 'none' && matrix.feature == 'none' }}
run: |
cargo build --no-default-features --features ${{ matrix.layout }}
cargo test --no-default-features --features ${{ matrix.layout }}
- if: ${{ matrix.layout == 'none' && matrix.feature != 'none' }}
run: |
cargo build --no-default-features --features ${{ matrix.feature }}
cargo test --no-default-features --features ${{ matrix.feature }}
- if: ${{ matrix.layout != 'none' && matrix.feature != 'none' }}
run: |
cargo build --no-default-features --features ${{ matrix.layout }},${{ matrix.feature }}
cargo test --no-default-features --features ${{ matrix.layout }},${{ matrix.feature }}
Copy link
Collaborator Author

@Weibye Weibye Apr 10, 2023

Choose a reason for hiding this comment

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

I find all this branching a bit clunky but it was the only way I've found so far to be able to specify "nothing". Maybe we don't need to test things like --no-default-features --features alloc?

If we remove the idea of testing for no layout feature together with optional, and extract cargo test --no-default-features as a separate job, we can remove all this branching.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the serde feature is possibly the one that ought to be pulled out into it's own job(s). I think alloc definitely needs to be tested. If anything it's the combinations that don't have either std or alloc that we could drop if want. I'm not sure how much sense it really makes to use Taffy in an environment without allocation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've put the allocation features: std & alloc into its own axis in the matrix. It will now always test with either of them.
I've also extracted cargo test --no-default-features into its own job.

Let me know what you think of the current setup

@Weibye Weibye added the build system Make continuous integration do the tedious things label Apr 10, 2023
@Weibye Weibye marked this pull request as draft April 10, 2023 12:06
@Weibye Weibye marked this pull request as ready for review April 10, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system Make continuous integration do the tedious things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants