Skip to content

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.

@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