tests: simplify random_fe_non_zero (remove loop limit and unneeded normalize) #1395

1. 
   theStack commented at 0:51 am on August 7, 2023: contributor

   random_fe_non_zero contains a loop iteration limit that ensures that we abort if random_fe ever yielded zero more than ten times in a row. This construct was first introduced in PR #19 (commit 09ca4f32) for random non-square field elements and was later refactored into the non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the exhaustive tests happened recently in PR #1118 (commit 0f864207).

   This case seems to be practically irrelevant and I’d argue for keeping things simple and removing it (which was already suggested in #1118 (review)); if there’s really a worry that the test’s random generator is heavily biased towards certain values or value ranges then there should consequently be checks at other places too (e.g. directly in random_fe for 256-bit values that repeatedly overflow, i.e. >= p).

   Also, the _fe_normalize call is not needed and can be removed, as the result of random_fe is already normalized.

2. real-or-random approved
3. 
   real-or-random commented at 1:13 am on August 7, 2023: contributor

   utACK 37974c5e007ba70056dc41e61f6c226075305c4f
4. real-or-random added the label refactor/smell on Aug 7, 2023
5. 
   real-or-random commented at 1:16 am on August 7, 2023: contributor

   To get rid of the duplication, we could move the ge_equals_* functions to group (I guess it doesn’t hurt to have them in the code) and the random_fe to testrand. Or we could just move everything to a new testutil.h.
6. 
   theStack commented at 5:37 pm on August 17, 2023: contributor

   To get rid of the duplication, we could move the ge_equals_* functions to group (I guess it doesn’t hurt to have them in the code) and the random_fe to testrand. Or we could just move everything to a new testutil.h.

   Both approaches sound reasonable to me, but I’ve decided to go with the second one and added a commit introducing a new testutil.h file. Right now only the duplicate functions (random_fe{,_non_zero and ge_equals_{ge,gej}) are moved, in the future it maybe makes sense to also move others that are similar (gej_xyz_equals_gej, random_fe_non_square etc.).

7. theStack force-pushed on Aug 17, 2023
8. tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize)

   `random_fe_non_zero` contains a loop iteration limit that ensures that
   we abort if `random_fe` ever yielded zero more than ten times in a row.
   This construct was first introduced in PR #19 (commit 09ca4f32) for
   random non-square field elements and was later refactored into the
   non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the
   exhaustive tests happened recently in PR #1118 (commit 0f864207).
   
   This case seems to be practically irrelevant and I'd argue for keeping
   things simple and removing it; if there's really a worry that the test's
   random generator is heavily biased towards certain values or value
   ranges then there should consequently be checks at other places too
   (e.g. directly in `random_fe` for 256-bit values that repeatedly
   overflow, i.e. >= p).
   
   Also, the _fe_normalize call is not needed and can be removed, as the
   result of `random_fe` is already normalized.

   dc5514144f
9. refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) c45b7c4fbb
10. theStack force-pushed on Aug 17, 2023
11. real-or-random approved
12. 
    real-or-random commented at 12:56 pm on September 13, 2023: contributor

    utACK c45b7c4fbbf41b011f138c465a58322a36664fd3
13. 
    siv2r commented at 12:48 pm on September 14, 2023: contributor

    ACK c45b7c4 (reviewed the changes and tests for both the commits passed locally).
14. real-or-random merged this on Sep 14, 2023
15. real-or-random closed this on Sep 14, 2023

    ---

16. theStack deleted the branch on Sep 14, 2023

Contributors
theStack real-or-random siv2r

Labels
refactor/smell