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

test: improve NAF decomposition test coverage #617

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

DeVikingMark
Copy link

Description:

Enhance TestNafDecomposition with comprehensive test cases:

  • Add test table with various input numbers
  • Add boundary cases (0, 1)
  • Add interesting cases requiring NAF conversion
  • Add property checks for NAF representation
  • Add reconstruction validation
  • Improve error messages with detailed information

This commit removes the TODO comment and provides proper test coverage for the NafDecomposition function, ensuring correct NAF properties and accurate number reconstruction.

Copy link
Collaborator

@yelhousni yelhousni left a comment

Choose a reason for hiding this comment

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

Thanks @DeVikingMark for the test!
There are though two sign errors in 15 and 31 and endianness error in 7. You should also run go fmt utils_test.go to pass CI.

{"13", []int8{1, 0, -1, 0, 1}}, // existing test case
{"0", []int8{}}, // edge case - zero
{"1", []int8{1}}, // edge case - one
{"7", []int8{1, 0, 0, -1}}, // 7 = 2³ - 2⁰ (8 - 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be {"7", []int8{-1, 0, 0, 1}}

{"0", []int8{}}, // edge case - zero
{"1", []int8{1}}, // edge case - one
{"7", []int8{1, 0, 0, -1}}, // 7 = 2³ - 2⁰ (8 - 1)
{"15", []int8{1, 0, 0, 0, 1}}, // 15 = 2⁴ - 2⁰
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be {"15", []int8{-1, 0, 0, 0, 1}}

{"1", []int8{1}}, // edge case - one
{"7", []int8{1, 0, 0, -1}}, // 7 = 2³ - 2⁰ (8 - 1)
{"15", []int8{1, 0, 0, 0, 1}}, // 15 = 2⁴ - 2⁰
{"31", []int8{1, 0, 0, 0, 0, 1}}, // 31 = 2⁵ - 2⁰
Copy link
Collaborator

Choose a reason for hiding this comment

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

it should be {"31", []int8{-1, 0, 0, 0, 0, 1}}

@yelhousni
Copy link
Collaborator

Thanks for the test! There are though two sign errors in 15 and 31 and endianness error in 7. You should also run go fmt utils_test.go to pass CI.

Here is the suggested diff:

diff --git a/ecc/utils_test.go b/ecc/utils_test.go
index fed45efe5..187d760df 100644
--- a/ecc/utils_test.go
+++ b/ecc/utils_test.go
@@ -12,12 +12,12 @@ func TestNafDecomposition(t *testing.T) {
 		input    string // large number in decimal form
 		expected []int8 // expected NAF representation
 	}{
-		{"13", []int8{1, 0, -1, 0, 1}},          // existing test case
-		{"0", []int8{}},                          // edge case - zero
-		{"1", []int8{1}},                         // edge case - one
-		{"7", []int8{1, 0, 0, -1}},              // 7 = 2³ - 2⁰ (8 - 1)
-		{"15", []int8{1, 0, 0, 0, 1}},           // 15 = 2⁴ - 2⁰
-		{"31", []int8{1, 0, 0, 0, 0, 1}},        // 31 = 2⁵ - 2⁰
+		{"13", []int8{1, 0, -1, 0, 1}},    // existing test case
+		{"0", []int8{}},                   // edge case - zero
+		{"1", []int8{1}},                  // edge case - one
+		{"7", []int8{-1, 0, 0, 1}},        // 7 = 2³ - 2⁰ (8 - 1)
+		{"15", []int8{-1, 0, 0, 0, 1}},    // 15 = 2⁴ - 2⁰
+		{"31", []int8{-1, 0, 0, 0, 0, 1}}, // 31 = 2⁵ - 2⁰
 	}
 
 	for i, test := range tests {
@@ -33,7 +33,7 @@ func TestNafDecomposition(t *testing.T) {
 
 		// Length check
 		if len(naf) != len(test.expected) {
-			t.Errorf("Test %d: Incorrect length for input %s. Got %d, want %d", 
+			t.Errorf("Test %d: Incorrect length for input %s. Got %d, want %d",
 				i, test.input, len(naf), len(test.expected))
 			continue
 		}
@@ -69,7 +69,7 @@ func TestNafDecomposition(t *testing.T) {
 			power.Mul(power, big.NewInt(2))
 		}
 		if reconstructed.Cmp(input) != 0 {
-			t.Errorf("Test %d: NAF reconstruction failed for input %s. Got %s", 
+			t.Errorf("Test %d: NAF reconstruction failed for input %s. Got %s",
 				i, test.input, reconstructed.String())
 		}
 	}

and a python code to cross check values:

def NAF(x):
	if x == 0:
		return []
	z = 0 if x % 2 == 0 else 2 - (x % 4)
	return NAF( (x-z) // 2 ) + [z]

@DeVikingMark
Copy link
Author

@yelhousni fixed, is it fine?

@ivokub ivokub requested a review from yelhousni February 13, 2025 17:06
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