Skip to content

Conversation

@maribu
Copy link
Member

@maribu maribu commented Nov 29, 2025

Contribution description

This hacks in some unit tests that will narrow down what part of uECC failed, if the test fails.

Not sure if we actually want this. It has been useful to narrow down what was causing the AVR test to fail, though.

Testing procedure

The test should still pass, but provide a bit more output.

Issues/PRs references

This was helpful for #21920

This hacks in some unit tests that will narrow down what part of uECC
failed, if the test fails.
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Nov 29, 2025
@crasbe crasbe added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Dec 1, 2025
@riot-ci
Copy link

riot-ci commented Dec 1, 2025

Murdock results

✔️ PASSED

cd5e8cc tests/pkg/micro-ecc: add unit tests

Success Failures Total Runtime
27 0 27 01m:54s

Artifacts

@AnnsAnns
Copy link
Member

AnnsAnns commented Dec 1, 2025

Not sure if we actually want this.

Why would we not want this?

@maribu
Copy link
Member Author

maribu commented Dec 1, 2025

It contains a nasty hack to access an internal function for unit testing.

There are btw also tests upstream: https://github.com/kmackay/micro-ecc/blob/master/test/test_compress.c

They won't catch any issues in our integration, though. We could try to PR changes to the upstream tests that would allow embedding them into an external application - such as a RIOT app - and then do that instead.

@AnnsAnns
Copy link
Member

AnnsAnns commented Dec 1, 2025

I mean IMO that looks fine to me. Clearly they left the access there for such cases, given that they also use it but I get your point

@maribu
Copy link
Member Author

maribu commented Dec 1, 2025

One benefit of this is that a catastrophic bug that would cause an all zero shared key to be generated would be caught, but not by the "do both parties arrive at the same shared key" test that follows.

Copy link
Member

@AnnsAnns AnnsAnns left a comment

Choose a reason for hiding this comment

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

I'm in favour of adding this, I see nothing wrong with having a backup test for this. But I'm open to other opinions.

Also, a few long lines in there ;)

};
uint8_t alignas(uECC_word_t) result_got[sizeof(result_expected)];
uECC_vli_modMult_fast((void *)result_got, (void *)input1, (void *)input2, curve);
return _test_compare("uECC_vli_modMult_fast()", result_got, result_expected, sizeof(result_expected));
Copy link
Member

Choose a reason for hiding this comment

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

Long line

};
uint8_t alignas(uECC_word_t) result_got[sizeof(result_expected)];
uECC_vli_modSquare_fast((void *)result_got, (void *)input, curve);
return _test_compare("uECC_vli_modSquare_fast()", result_got, result_expected, sizeof(result_expected));
Copy link
Member

Choose a reason for hiding this comment

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

long line

Comment on lines +163 to +164
uECC_vli_mult((void *)result_got, (void *)input1, (void *)input2, sizeof(input1) / sizeof(uECC_word_t));
return _test_compare("uECC_vli_mult()", result_got, result_expected, sizeof(result_expected));
Copy link
Member

Choose a reason for hiding this comment

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

Long lines

Comment on lines +189 to +191
uECC_word_t carry_got = uECC_vli_add((void *)result_got, (void *)input1, (void *)input2, sizeof(input1)/sizeof(uECC_word_t));
return _test_compare("uECC_vli_add()", result_got, result_expected, sizeof(result_expected))
&& _test_compare("uECC_vli_add() carry", &carry_got, &carry_expected, sizeof(carry_expected));
Copy link
Member

Choose a reason for hiding this comment

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

long lines

};
const uECC_word_t borrow_expected = 1;
uint8_t alignas(uECC_word_t) result_got[sizeof(result_expected)];
uECC_word_t borrow_got = uECC_vli_sub((void *)result_got, (void *)input1, (void *)input2, sizeof(input1)/sizeof(uECC_word_t));
Copy link
Member

Choose a reason for hiding this comment

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

long line

Comment on lines +248 to +249
uECC_vli_mmod((void *)result_got, (void *)product, (void *)mod, sizeof(mmod_result_expected) / sizeof(uECC_word_t));
return _test_compare("uECC_vli_mmod()", result_got, mmod_result_expected, sizeof(mmod_result_expected));
Copy link
Member

Choose a reason for hiding this comment

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

Long lines

alignas(uECC_word_t) uint8_t product[sizeof(mmod_input)];
memcpy(product, mmod_input, sizeof(product));
curve->mmod_fast((void *)result_got, (void *)product);
return _test_compare("curve->mmod_fast()", result_got, mmod_result_expected, sizeof(mmod_result_expected));
Copy link
Member

Choose a reason for hiding this comment

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

long line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants