Skip to content

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

Merged
cpu merged 2 commits into
rustls:mainfrom
cpu:cpu-adopt-some-coverage
May 26, 2023
Merged

tests: misc coverage for calendar.rs, der.rs#71
cpu merged 2 commits into
rustls:mainfrom
cpu:cpu-adopt-some-coverage

Conversation

@cpu

@cpu cpu commented May 25, 2023

Copy link
Copy Markdown
Member

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

cpu commented May 25, 2023

Copy link
Copy Markdown
Member Author

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

codecov Bot commented May 25, 2023

Copy link
Copy Markdown

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

djc commented May 25, 2023

Copy link
Copy Markdown
Member

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

cpu commented May 25, 2023

Copy link
Copy Markdown
Member Author

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

djc commented May 25, 2023

Copy link
Copy Markdown
Member

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

cpu commented May 25, 2023

Copy link
Copy Markdown
Member Author

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

djc commented May 25, 2023

Copy link
Copy Markdown
Member

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

Comment thread src/calendar.rs Outdated
Comment thread src/der.rs Outdated
@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