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.

Sign up for GitHub

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

Jump to bottom

tests: Tidy the util functions #1491

Closed
real-or-random opened this issue Feb 1, 2024 · 2 comments
Closed

tests: Tidy the util functions #1491

real-or-random opened this issue Feb 1, 2024 · 2 comments
Labels
assurance refactor/smell

Comments

@real-or-random
Copy link
Contributor

When I look at all the helper functions near the top of tests.c, there are still some things that could be improved:

  • A lot of these functions could be moved to testutil or testrand, also functions like random_fe, random_fe_magnitude. They're currently only in tests.c but they're utils in the end.
  • We have functions with group_element and field_element, but they should just be ge and fe.
  • The ones in testutil should probably also get a secp256k1_testutil prefix then. You could say it's good that they stand out currently, but at least that's inconsistent with the ones in testrand, which have a prefix. Or perhaps better: we use prefixes testutil_ and testrand_, dropping the additional secp256k1_ for test functions. Then they still stand out. (The main purpose of secp256k1_ is to avoid namespace collisions if someone uses the library by just including it, but that doesn't matter in test-only code.)

Noticed when looking into #1489.
@theStack theStack mentioned this issue May 27, 2024
Merged
real-or-random added a commit that referenced this issue Jun 12, 2024
@real-or-random
Merge #1533: tests: refactor: tidy up util functions (#1491)
4392f0f 
e73f6f8 tests: refactor: drop `secp256k1_` prefix from testrand.h functions (Sebastian Falbesoner)
0ee7453 tests: refactor: add `testutil_` prefix to testutil.h functions (Sebastian Falbesoner)
0c6bc76 tests: refactor: move `random_` helpers from tests.c to testutil.h (Sebastian Falbesoner)
0fef847 tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude` (Sebastian Falbesoner)
59db007 tests: refactor: rename `random_group_element_...` -> `random_ge_...` (Sebastian Falbesoner)

Pull request description:

  This PR is an attempt at tidying up test util functions, as suggested in #1491. The following changes are done:
  * rename `_group_element...` functions to `_ge...`
  * rename `_field_element...` functions to `_fe...`
  * move `random_` helpers from tests.c to testutil.h (the alternative would be testrand.h, but to my understanding, this one is meant to contain the actual RNG implementation rather than helpers using it; happy to move the helpers there if that is preferred though)
  * prefix testutil.h functions with `testutil_`
  * prefix testrand.h functions with `testrand_` (this is currently done in a sloppy way by simply dropping the `secp256k1_` prefix, so some functions don't have the full prefix, like e.g. `testrand256`; naming suggestions welcome)

ACKs for top commit:
  sipa:
    utACK e73f6f8
  real-or-random:
    utACK e73f6f8

Tree-SHA512: c87a35a9f7f23d4bbb87a1ff0d40dd5fbd7d976719ca1027cad187ac44aa2db3ae887ac620639d2287c260e701a5963830b52048692d3e6b38b5eb6cdf17b854
@fanquake
Copy link
Member

What is left to do here post #1533 ?

@real-or-random
Copy link
Contributor Author

What is left to do here post #1533 ?

Nothing, thanks for the reminder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
assurance refactor/smell
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fanquake @real-or-random