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

tests: misc coverage for calendar.rs, der.rs #71

Merged
merged 2 commits into from
May 26, 2023
Merged

tests: misc coverage for calendar.rs, der.rs #71

merged 2 commits into from
May 26, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented May 25, 2023

I was looking at some of the open PRs on the original briansmith/webpki repo and spotted two small ones adding bits of test coverage:

The unit tests look good to me and more test coverage seems appealing so I've cherry-picked the original commits onto our fork and squashed each down into 1 commit per area of coverage (and fixed a couple clippy errs).

Thanks @st3fan for the original implementation! :-)

@cpu cpu self-assigned this May 25, 2023
@cpu
Copy link
Member Author

cpu commented May 25, 2023

ci / clippy (pull_request) Failing after 14s

Ah! I keep forgetting cargo clippy locally doesn't match the CI config. I'll fix these up in a jiffy.

@codecov
Copy link

codecov bot commented May 25, 2023

Codecov Report

Merging #71 (b2756e0) into main (e282ccb) will increase coverage by 0.25%.
The diff coverage is 100.00%.

❗ Current head b2756e0 differs from pull request most recent head 882d929. Consider uploading reports for the commit 882d929 to get more accurate results

@@            Coverage Diff             @@
##             main      #71      +/-   ##
==========================================
+ Coverage   95.08%   95.34%   +0.25%     
==========================================
  Files          13       13              
  Lines        2604     2684      +80     
==========================================
+ Hits         2476     2559      +83     
+ Misses        128      125       -3     
Impacted Files Coverage Δ
src/calendar.rs 95.20% <100.00%> (+2.47%) ⬆️
src/der.rs 96.64% <100.00%> (+2.57%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@djc
Copy link
Member

djc commented May 25, 2023

ci / clippy (pull_request) Failing after 14s

Ah! I keep forgetting cargo clippy locally doesn't match the CI config. I'll fix these up in a jiffy.

I forget, why is that again?

(https://twitter.com/weihanglo/status/1661680417643143169 should help, eventually.)

@cpu
Copy link
Member Author

cpu commented May 25, 2023

I forget, why is that again?

There's a helper script I keep forgetting to use, (mk/clippy.sh) that sets a handful of flags:

webpki/mk/clippy.sh

Lines 21 to 35 in 6a388ac

cargo clippy \
--target-dir=target/clippy \
--all-features --all-targets \
-- \
--deny missing_docs \
--deny warnings \
\
--deny clippy::as_conversions \
\
--allow clippy::len_without_is_empty \
--allow clippy::new_without_default \
--allow clippy::single_match \
--allow clippy::single_match_else \
--allow clippy::type_complexity \
--allow clippy::upper_case_acronyms \

(https://twitter.com/weihanglo/status/1661680417643143169 should help, eventually.)

Oh cool. That will be a nice feature.

@djc
Copy link
Member

djc commented May 25, 2023

Right, I meant, I forget why we don't have those in the crate root as inner attributes like we do for other stuff.

@cpu
Copy link
Member Author

cpu commented May 25, 2023

Right, I meant, I forget why we don't have those in the crate root as inner attributes like we do for other stuff.

Ahhh, I understand. Some git spelunking suggests there used to be crate attributes for clippy config and they were lifted out into the helper in briansmith/webpki@2799332 as part of synchronizing with ring. I can't read between the lines to determine the rationale for that 🤔

@djc I could open a separate PR to move back to that approach if you think it's worthwhile. It would reduce some dev. friction 👍

@djc
Copy link
Member

djc commented May 25, 2023

Yeah, I think that would be an improvement. I'd probably just get rid of the helper script.

src/calendar.rs Outdated Show resolved Hide resolved
src/der.rs Outdated Show resolved Hide resolved
@cpu cpu merged commit b4f29f4 into rustls:main May 26, 2023
@cpu cpu deleted the cpu-adopt-some-coverage branch May 26, 2023 12:13
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.

3 participants