Status

NSS
Test
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: fkiefer, Assigned: fkiefer)

Tracking

(Blocks: 2 bugs)

trunk
3.29
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected)

Details

Attachments

(2 attachments, 1 obsolete attachment)

There are ecl tests in lib/freebl/ecl/tests that are never run (and that don't compile at the moment). We should rewrite those tests as gtests to get test coverage for ecc code.
Blocks: 1253911
Alias: ecc-tests
Created attachment 8756402 [details] [diff] [review]
enable-more-ectests.patch

this adds some basic ec tests for prime fields that have been previously disabled.

In order to make this work I added a new environment variable FREEBL_TEST that exposes all freebl functionality. This will be useful for future testing of freebl in general (e.g. bug 1225323).
Assignee: nobody → franziskuskiefer
Attachment #8756402 - Flags: review?(ttaubert)
Attachment #8756402 - Flags: review?(rrelyea)
Attachment #8756402 - Flags: review?(kaie)
Created attachment 8756430 [details] [diff] [review]
enable-more-ectests.patch v2

The tests are now disabled when run through all.sh if FREEBL_TEST is not set.
Attachment #8756402 - Attachment is obsolete: true
Attachment #8756402 - Flags: review?(ttaubert)
Attachment #8756402 - Flags: review?(rrelyea)
Attachment #8756402 - Flags: review?(kaie)
Attachment #8756430 - Flags: review?(ttaubert)
Attachment #8756430 - Flags: review?(rrelyea)
Attachment #8756430 - Flags: review?(kaie)
Comment on attachment 8756430 [details] [diff] [review]
enable-more-ectests.patch v2

Review of attachment 8756430 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. cmd/ec-tests/manifest.mn looks ok, but I'm not too familiar with this stuff.

::: tests/ec/ec-tests.sh
@@ +21,4 @@
>  # local shell function to initialize this script
>  ########################################################################
>  
> +ec-tests_init()

Didn't even know that was allowed as a function name, but maybe change this to ec_tests_init()? Same for the other functions.
Attachment #8756430 - Flags: review?(ttaubert) → review+

Comment 4

2 years ago
Created attachment 8763858 [details] [diff] [review]
ectest-w.patch

this is the same patch as before, but ignoring whitespace changes.

Comment 5

2 years ago
It took me a while to understand the intentions here, thanks for answering my questions on IRC.

You intend to use a special build mode of NSS, which exports internal APIs, to allow you to access that internal functionality from special test code.

If we want to support such a test mode, I suggest to use a more general build flag for that purpose. Instead of FREEBL_TEST, for example NSS_EXPORT_PRIVATE_APIS ?


I tried your patch, with FREEBL_TEST=1, but it didn't build the ec-tests binary.

I added it locally, and the build fails with unresolved externals, apparently the variable isn't yet passed down to the scripts and makefiles? Please attach a patch that works.


As a general improvement to both your new code, and to the existing ecperf.sh script, I'd like to suggest the following addition:

 ECPERF_OUT=`ecperf`
+echo $ECPERF_OUT
 ECPERF_OUT=`echo $ECPERF_OUT | grep -i "failed"`

When reading the full test output.log, this will make it easier to understand that the tests is indeed executed, and to see the output.

Comment 6

2 years ago
Comment on attachment 8756430 [details] [diff] [review]
enable-more-ectests.patch v2

I think this needs more work.
Attachment #8756430 - Flags: review?(kaie) → review-
We have fbectest and pk11ectest so these tests are not needed anymore.
https://hg.mozilla.org/projects/nss/rev/949bab37f7486b052b166368c7fa3abc4a688414
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → 3.29
You need to log in before you can comment on or make changes to this bug.