Skip to content

Conversation

korjeek
Copy link
Contributor

@korjeek korjeek commented Sep 20, 2025

Adding comprehensive unit tests for SHA-256 implementation.

These tests verify:

  1. Basic functionality with known test vectors (e.g., "abc")
  2. Handling of large input data (4KB repeated 1000 times)
  3. Edge case with repeated single-byte input (1 million 'a' characters)

The tests ensure compatibility with standard SHA-256 implementations and will help detect regressions during future code changes.

Copy link

codecov bot commented Sep 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.23%. Comparing base (8d562d2) to head (90f2407).
⚠️ Report is 1 commits behind head on unstable.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #2632      +/-   ##
============================================
+ Coverage     72.22%   72.23%   +0.01%     
============================================
  Files           127      127              
  Lines         70936    70936              
============================================
+ Hits          51232    51244      +12     
+ Misses        19704    19692      -12     

see 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

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

Looks fine. I am not against adding these tests (maybe they can help in a future need to change the sha256 implementation...) biut can you please through some words on the top comment to explain what you are adding and why?

@ranshid
Copy link
Member

ranshid commented Sep 21, 2025

Thank you @korjeek I meant the top comment :) , but adding these comments in the test file also works.
I edited the top comment as well. the reason is that the top comment is used as the final commit message. it is better to have some content attached to each commit.

@ranshid ranshid self-requested a review September 21, 2025 13:35
@ranshid
Copy link
Member

ranshid commented Sep 21, 2025

@korjeek can you PTAL at the test failures and fix?

@korjeek
Copy link
Contributor Author

korjeek commented Sep 21, 2025

@korjeek can you PTAL at the test failures and fix?

seems like I fixed it

@ranshid ranshid merged commit 298f7da into valkey-io:unstable Sep 21, 2025
52 checks passed
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.

2 participants