Closed Bug 1253910 (ecc-tests) Opened 8 years ago Closed 7 years ago

ECC tests

Categories

(NSS :: Test, defect)

defect
Not set
normal

Tracking

(firefox47 affected)

RESOLVED FIXED
Tracking Status
firefox47 --- affected

People

(Reporter: franziskus, Assigned: franziskus)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 1 obsolete file)

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: ec-cleanup
Alias: ecc-tests
Attached patch enable-more-ectests.patch (obsolete) — Splinter Review
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)
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+
Attached patch ectest-w.patchSplinter Review
this is the same patch as before, but ignoring whitespace changes.
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 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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: