Add testing whitelist #143

jeremylt · 2018-08-26T21:12:12Z

This adds a testing whitelist to make backend developers explicitly list which tests they will not support

jeremylt · 2018-08-26T21:24:07Z

@dmed256 @camierjs This branch is failing the OCCA tests on travis but passing on my local machine. I suspect that something changed in OCCA, as this branch did not change any OCCA files and the OCCA backend was passing tests last night on Travis.

jeremylt · 2018-08-26T21:30:36Z

tests/tap.sh

-            ${output}.err; then
+     # grep to skip test if backend chooses to whitelist test
+    if grep -F -q -e 'Backend does not support' ${output}.err \
+             && grep -F -q -e "$1 $backend" tests/whitelist ; then


I think it is a safer policy to force backends to explicitly list what functions they do not support so we don't get false passes from the test suite

Could we make this something that a backend implements instead of residing in a common whitelist (that will need to be edited and merged)? One solution would be for the backend to implement the function that it "does not support", but raise an error with a different syntax to indicate that it is intentional.

How about this change, backends add "backend does not implement..."? If we like it I can squash the two commits.

jeremylt · 2018-08-26T21:32:15Z

tests/whitelist

+t202-elemrestriction-f /gpu/occa
+t202-elemrestriction-f /omp/occa
+t202-elemrestriction-f /ocl/occa
+t202-elemrestriction-f /gpu/magma


I am listing the Fortran and C tests separately. I did it this way because we have had trouble with the Fortran tests not being the same as the C tests and certain backends not playing nice over Fortran.

jeremylt · 2018-08-26T21:33:43Z

tests/tap.sh

@@ -53,15 +53,15 @@ for ((i=0;i<${#backends[@]}; ++i)); do

    # grep to pass test t103 on error
    if grep -F -q -e 'Cannot grant CeedVector array access, the access lock is already in use' \
-            ${output}.err; then
+            ${output}.err && [[ "$1" = "t103"* ]] ; then


I explicitly added this check so we don't get false positives on other tests.

jeremylt · 2018-08-27T01:54:08Z

This fails in Travis but passes with OCCA libocca/occa@527494c

jedbrown

I assume we want SKIPs to be tallied.

jedbrown · 2018-08-27T02:52:59Z

backends/magma/ceed-magma.c

+    CeedMemType mtype,
+    CeedCopyMode cmode,
+    const CeedInt *indices) {
+  return CeedError(r->ceed, 1, "Backend does not impliment blocked restrictions");


jedbrown · 2018-08-27T02:54:28Z

tests/tap.sh

-        printf "ok $i0 # SKIP $1 $backend\n"
-        printf "ok $i1 # SKIP $1 $backend stdout\n"
-        printf "ok $i2 # SKIP $1 $backend stderr\n"
+        printf "ok $i0 # OCCA SKIP $1 $backend\n"


I believe TAP says the SKIP must come first.

Correct, fixed

jeremylt · 2018-08-27T03:14:15Z

Huh, I had thought TAP already tallied skips, but I guess not

jeremylt · 2018-08-27T03:59:38Z

Perl might have intentionally moved away from explicitly listing the number of skipped tests in the summary result:
https://grokbase.com/t/perl/qa/092jekprma/test-skip-ing-weirdness

Currently, Perl will output the number of TODO tests, but not the number of SKIP tests. I changed the directives of the tests where functions are intentionally not enabled to TODO. I am not wild about the label TODO as some backends may intentionally never implement a function, but I think it is important to differentiate between an 'OCCA backend not supported' skip and a 'functionality not implemented' skip.

jedbrown · 2018-08-27T13:00:53Z

tests/tap.sh

+    if grep -F -q -e 'Backend does not implement' \
+            ${output}.err ; then
+        printf "ok $i0 # TODO - not implemented $1 $backend\n"
+        printf "ok $i1 # TODO - not implemented $1 $backend stdout\n"


Specification says TODO should be marked not ok. I don't have a problem with prove's behavior, but I think we should try to follow the standard when using directives. There are other TAP consumers including Jenkins, which we might use to get CI on GPU or other DOE hardware/software stacks.

I mixed it up; I thought TODO was another type of conditional pass. Fixed now.

Explicitly check for t103 or t104 for expected fail

codecov · 2018-08-27T18:32:34Z

Codecov Report

Merging #143 into master will decrease coverage by 0.04%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   90.68%   90.64%   -0.05%     
==========================================
  Files          77       77              
  Lines        3931     3934       +3     
==========================================
+ Hits         3565     3566       +1     
- Misses        366      368       +2

Flag	Coverage Δ
#backends	`88.11% <33.33%> (-0.09%)`	⬇️
#examples	`87.69% <ø> (ø)`	⬆️
#interface	`89.78% <ø> (ø)`	⬆️
#tests	`96.64% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
backends/occa/ceed-occa-restrict.c	`91.73% <0%> (-1.55%)`	⬇️
backends/occa/ceed-occa.c	`71.27% <100%> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 269c27c...4cc6aec. Read the comment docs.

jeremylt · 2018-08-27T20:00:47Z

Code coverage went down because lines that return an error code that gets the test listed as skipped are not counted as covered. Otherwise this build passes.

jedbrown · 2018-08-27T21:06:10Z

We aren't slaves to the coverage overlords.

jeremylt · 2018-08-27T21:16:18Z

I wanted to at least set as a norm that we acknowledge and understand why coverage goes down and then we can overrule the codecov check if it makes sense.

jeremylt requested a review from jedbrown August 26, 2018 21:12

jeremylt force-pushed the test-whitelist branch 2 times, most recently from a012494 to 4cdcc9e Compare August 26, 2018 21:20

jeremylt commented Aug 26, 2018

View reviewed changes

jeremylt mentioned this pull request Aug 26, 2018

libCEED Travis CI With OCCA Now Failing libocca/occa#198

Closed

jeremylt added the testing label Aug 26, 2018

jeremylt force-pushed the test-whitelist branch 2 times, most recently from 7d67262 to ddc8744 Compare August 26, 2018 23:34

jedbrown reviewed Aug 27, 2018

View reviewed changes

jeremylt force-pushed the test-whitelist branch from ddc8744 to 5724b82 Compare August 27, 2018 03:10

jeremylt force-pushed the test-whitelist branch from 5724b82 to c750d1e Compare August 27, 2018 04:45

jedbrown reviewed Aug 27, 2018

View reviewed changes

jeremylt force-pushed the test-whitelist branch from c750d1e to 48fecdc Compare August 27, 2018 13:42

Add testing whitelist

4cc6aec

Explicitly check for t103 or t104 for expected fail

jeremylt force-pushed the test-whitelist branch from 48fecdc to 4cc6aec Compare August 27, 2018 18:14

jedbrown approved these changes Aug 27, 2018

View reviewed changes

jeremylt merged commit 5726b88 into master Aug 27, 2018

jeremylt deleted the test-whitelist branch August 28, 2018 22:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add testing whitelist #143

Add testing whitelist #143

jeremylt commented Aug 26, 2018

jeremylt commented Aug 26, 2018 •

edited

jeremylt Aug 26, 2018

jedbrown Aug 26, 2018

jeremylt Aug 26, 2018

jeremylt Aug 26, 2018

jeremylt Aug 26, 2018

jeremylt commented Aug 27, 2018

jedbrown left a comment

jedbrown Aug 27, 2018

jedbrown Aug 27, 2018

jeremylt Aug 27, 2018

jeremylt commented Aug 27, 2018

jeremylt commented Aug 27, 2018 •

edited

jedbrown Aug 27, 2018

jeremylt Aug 27, 2018

codecov bot commented Aug 27, 2018 •

edited

jeremylt commented Aug 27, 2018 •

edited

jedbrown commented Aug 27, 2018

jeremylt commented Aug 27, 2018

Add testing whitelist #143

Add testing whitelist #143

Conversation

jeremylt commented Aug 26, 2018

jeremylt commented Aug 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylt commented Aug 27, 2018

jedbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremylt commented Aug 27, 2018

jeremylt commented Aug 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 27, 2018 • edited

Codecov Report

jeremylt commented Aug 27, 2018 • edited

jedbrown commented Aug 27, 2018

jeremylt commented Aug 27, 2018

jeremylt commented Aug 26, 2018 •

edited

jeremylt commented Aug 27, 2018 •

edited

codecov bot commented Aug 27, 2018 •

edited

jeremylt commented Aug 27, 2018 •

edited