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

Simplify CI testing matrix #673

Merged
merged 2 commits into from Oct 18, 2021
Merged

Simplify CI testing matrix #673

merged 2 commits into from Oct 18, 2021

Conversation

lnicola
Copy link
Member

@lnicola lnicola commented Oct 16, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Closes #672

@lnicola
Copy link
Member Author

lnicola commented Oct 16, 2021

This reduces the CI time from 26 to 6.5 minutes.

@rmanoka
Copy link
Contributor

rmanoka commented Oct 17, 2021

Should we also disable testing the proj-network feature or leave it as-is until it causes some test breaks?

@lnicola
Copy link
Member Author

lnicola commented Oct 17, 2021

I wanted to, but was worried we wouldn't test new features. But yeah, let's try it.

@lnicola
Copy link
Member Author

lnicola commented Oct 17, 2021

Updated.

@rmanoka
Copy link
Contributor

rmanoka commented Oct 17, 2021

Lgtm! Let's see if anyone with more knowledge of the proj integration has some comments. Ping @michaelkirk @urschrei .

@urschrei
Copy link
Member

I'm trying to think of ways in which not testing the network functionality could cause an issue for geo, but so far I'm coming up blank: all the functionality enabled by the feature is in the proj crate, releases of which are gated on passing tests (which include testing network functionality). The feature builds our bundled version of proj, adding a build dependency on libtiff, as well as depending on 'reqwest'.

- run: cargo install cargo-all-features
- run: cargo build-all-features
- run: cargo test-all-features
- run: cargo check --all-targets --no-default-features
Copy link
Member

Choose a reason for hiding this comment

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

For posterity, can you add a comment as to why we're not doing --all-features here

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, added a comment.

@urschrei
Copy link
Member

Looks like we're down to ~7 minutes now, with the bench job taking the longest.

@michaelkirk
Copy link
Member

bors r+

Let's do it!

@bors
Copy link
Contributor

bors bot commented Oct 18, 2021

Build succeeded:

@bors bors bot merged commit a406c8d into master Oct 18, 2021
@lnicola lnicola deleted the trim-ci branch October 18, 2021 17:36
@michaelkirk
Copy link
Member

michaelkirk commented Oct 27, 2021

/cc @frewsxcv re: #671 (comment)

I just wanted to make sure that you saw this PR was merged.

In hindsight, I should have tagged you earlier, since I know you are especially interested in our testing strategy.

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.

RFC: Stop testing feature combinations exhaustively
4 participants