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

Adds CI matrix for crate features #533

Merged
merged 3 commits into from
Apr 26, 2023
Merged

Conversation

almann
Copy link
Contributor

@almann almann commented Apr 26, 2023

This change makes it so we build our crate with all the various interesting permutations:

  • default - build with no features set.
  • --all-features - build with all the bells and whistles.
  • --features X where X is:
    • ion-hash
    • experimental
    • ion-hash,experimental

When building/testing with these permutations, it uncovered a bug in the ion-hash doc tests where the test code depends on an optional feature. This has been conditionally compiled out, preserving the text of the doc comment (only runs when the appropriate feature flag is enabled).

Also updates the build of API docs to error out on leaking internal references. This change cleans up such documentation comments.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

This change makes it so we build our crate with all the various
interesting permutations:

* *default* - build with no features set.
* `--all-features` - build with all the bells and whistles.
* `--features X` where `X` is:
  * `ion-hash`
  * `experimental`
  * `ion-hash,experimental`

When building/testing with these permutations, it uncovered a bug in the
`ion-hash` doc tests where the test code depends on an optional feature.
This has been conditionally compiled out, preserving the text of the
doc comment (only runs when the appropriate feature flag is enabled).

Also updates the build of API docs to error out on leaking internal
references.  This change cleans up such documentation comments.
Copy link
Contributor

@popematt popematt left a comment

Choose a reason for hiding this comment

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

Should we check combinations of features for the coverage.yml workflow as well?

.github/workflows/rust.yml Outdated Show resolved Hide resolved
.github/workflows/rust.yml Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01 ⚠️

Comparison is base (4872ae8) 89.34% compared to head (f98c539) 89.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #533      +/-   ##
==========================================
- Coverage   89.34%   89.34%   -0.01%     
==========================================
  Files          84       84              
  Lines       13284    13284              
==========================================
- Hits        11869    11868       -1     
- Misses       1415     1416       +1     
Impacted Files Coverage Δ
src/ion_data/mod.rs 100.00% <ø> (ø)
src/ion_hash/mod.rs 100.00% <ø> (ø)
src/lib.rs 89.21% <ø> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Co-authored-by: Matthew Pope <81593196+popematt@users.noreply.github.com>
@almann
Copy link
Contributor Author

almann commented Apr 26, 2023

Should we check combinations of features for the coverage.yml workflow as well?

@popematt - I don't think this is worth it. For code coverage, I think we just want to see the aggregate sum of coverage via --all-features especially when the denominator is "all the code."

@almann
Copy link
Contributor Author

almann commented Apr 26, 2023

Closing and re-opening to try to unstuck Waiting for status to be reported

There is something odd going on here:
https://stackoverflow.com/questions/52200096/github-pull-request-waiting-for-status-to-be-reported

@almann almann closed this Apr 26, 2023
@almann almann reopened this Apr 26, 2023
@popematt
Copy link
Contributor

Closing and re-opening to try to unstuck Waiting for status to be reported

There is something odd going on here: https://stackoverflow.com/questions/52200096/github-pull-request-waiting-for-status-to-be-reported

Actually, it's waiting for those because of the branch protection rule. I just updated it, and as you can see, now it's waiting for e.g. Build and Test (macos-latest, all) instead of Build and Test (macos-latest). (Yes, it uses string matching on the workflow name.)

@almann
Copy link
Contributor Author

almann commented Apr 26, 2023

Actually, it's waiting for those because of the branch protection rule. I just updated it, and as you can see, now it's waiting for e.g. Build and Test (macos-latest, all) instead of Build and Test (macos-latest). (Yes, it uses string matching on the workflow name.)

Oh thanks! I was quite puzzled as to what I was missing...

@almann almann merged commit 448b1b2 into amazon-ion:main Apr 26, 2023
34 checks passed
@almann almann deleted the ci-features branch April 26, 2023 19:47
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

2 participants