search

exercism / common-lisp

Exercism exercises in Common Lisp.
https://exercism.org/tracks/common-lisp
MIT License
82 stars 77 forks source link

ABCL returns status 0 (success) when there is an error and it hasn't run any of the tests. #63

Closed wobh closed 9 years ago

wobh commented 9 years ago

ABCL returns status 0 (success) when there is an error and it hasn't run any of the tests.

What might be happening is that, after loading one of the "dna" example packages ("point-mutations", "nucleotide-count", or "rna-transcription") it treats subsequent defpackage calls as redefining the package, and removes any previous defined symbols.

This is should probably be considered a bug in "xlisp-test" which should treat the examples a fixtures and load and unload them around test runs. (It's still probably reasonable load all the test packages in advance, as they all have different names.)

In the meantime it's worrisome that an error like this should not fail the test suite. I don't know if there's some way to tell abcl or java to do this, or if we'll have to write an error catcher into the abcl command-line switch -e.

In the meantime, maybe we should set abcl builds into "allow failures" (even though the problem is, is that they're not failing when expected)

verdammelt commented 9 years ago

RE: your theory about DEFPACKAGE. I just took a quick look at the spec and there is perhaps enough wiggle room for that. "If the new definition is at variance with the current state of that package, the consequences are undefined" (http://www.lispworks.com/documentation/HyperSpec/Body/m_defpkg.htm).

verdammelt commented 9 years ago

ABCL throwing exception while reading test files. Will need to catch this error, print to *STANDARD-OUTPUT* and then exit.

Thinking of using HANDLER-CASE to wrap XLISP-TEST:RUN-TEST-ALL...

wobh commented 9 years ago

I'm refactoring that whole thing in #64. But it would be good to know what error it's having since, in addition to printing the error, you have to return nil. I was thinking we'd need something bonkers like:

(let (results condition)
  (unwind-protect
      (multiple-value-setq (results conditions)
        (ignore-errors #<...>))
    (progn
      (when (and (null results) (typep condition 'condition))
          (write-line condition))
      results)))

But that's bonkers. There's got to be a better way. I know handler-bind lets you do "catch and release" style error handling without unwinding stack, which is why I used it elsewhere. I worry that, it's not helpful in this case, and if it is, if I would end up with anything better than the above.

canweriotnow commented 9 years ago

Oh dear god, I'm understanding why Rich doesn't want reader-macros in Clojure :)

On Tue, Jun 16, 2015 at 10:59 PM William Clifford notifications@github.com wrote:

I'm refactoring that whole thing in #64 https://github.com/exercism/xlisp/issues/64. But it would be good to know what error it's having since, in addition to printing the error, you have to return nil. I was thinking we'd need something bonkers like:

(let (results condition) (unwind-protect (multiple-value-setq (results conditions) (ignore-errors #<...>)) (progn (when (and (null results) (typep condition condition)) (write-line condition)) results)))

But that's bonkers. There's got to be a better way. I know handler-bind lets you do "catch and release" style error handling without unwinding stack, which is why I used it elsewhere. I worry that, it's not helpful in this case, and if it is, if I would end up with anything better than the above.

— Reply to this email directly or view it on GitHub https://github.com/exercism/xlisp/issues/63#issuecomment-112631088.

wobh commented 9 years ago
  1. Doesn't he already have them? 2. I don't see it, but what about that expresses why reader macros would be problematic in Clojure to you?
verdammelt commented 9 years ago

@wobh ABCL is raising a READER-ERROR when loading the test files. Unclear on why you are saying it needs to return nil, maybe that is your new code? In the existing code I am playing with the idea of the handler reporting the error and returning the condition - which would cause RUN-TESTS-ALL to return non-nil which would cause the driver code to then exit with error code 4 (why 4 by the way?).

Your changes for #64 are loading the example, the test and then running them, then cleaning up, for each exercise right? That function could be wrapped in something to catch errors so it would return either a test result or a condition - then all those results could be reported on by the driver loop? (Does that make any sense? Maybe I ought to keep quiet and check out your work on the other ticket.)

wobh commented 9 years ago

You're right, I'm confused. I had waffled around whether to return t or nil when tests passed. I eventually landed on nil (consistent with assert) but I apparently waffled again thinking about this. Returning the condition is a good idea, and seems simpler to implement, but...

So, I had to distinguish exits due to problems while running the cl script, verses tests failing or raising errors. Errors in the script were exiting with 1. I looked up some exit codes to see what standards, conventions, and traditions were out there and there are a few, but none that I thought relevant. Out of the free space of exit codes (3 <= n < 64, or 79 <= n < 126) I chose 4 as the lowest small number that wouldn't be immediately used for something else. If 4 means that a test failed, it seems reasonable to make sure that a failure of this sort is detected and distinguished from the others. Thinking ahead, we could distinguish three conditions something like this:

This suggests a change around running tests that would have to be reflected in the cl script used in ".travis.yml". I'm thinking the -e option will have to become something like (uiop:quit (xlisp-test:run-tests-all)).

Anyway, don't keep quiet, I think it's good to talk about these things.

verdammelt commented 9 years ago

re: exit codes. thanks for giving me the background on why you chose 4. I'm not sure if I agree that we need different exit codes for different failure conditions - but as long as it doesn't twist up the code too much it might be useful in the future. Myself, I see tests failing due to exceptions, or assertion failures, or just failing to load/compile as all the same - it didn't work, I check the output to see why and I fix it.

Sounds like the function to load-example-load-test-run-test should return an data object which contains data about the result and can be queried about the outcome. Perhaps an CLOS object, perhaps just a closure, perhaps just a hash or alist. But even in the case of total success it should be returned so we don't have to worry about nil, or how to interpret it later.

verdammelt commented 9 years ago

@wobh Would it be useful for me to continue to get this working in the existing code given that you are reworking how tests are loaded/run in #64? Or should this issue be blocked from completion until that one is done?

wobh commented 9 years ago

If I were working on #63, I wouldn't feel blocked if you were working on #64 . I see them overlapping in a trivial way that I feel sure we can figure out with little trouble should a merge conflict arise (which is probably likely, I just changed the name of run-tests-all to test-exercises in addition to some other changes in ".travis.yml" that I probably should have waited on.)

In any case, I think it's almost done. I'm doing some testing this evening. I think it likely that I could submit a PR for it in the next few days unless something comes up. You can see what I've been up to here: https://github.com/wobh/xlisp/tree/xlisp-64-load-tests-singly

wobh commented 9 years ago

That went much better than I expected. If you look at this build fork all the tests pass https://travis-ci.org/wobh/xlisp and if you look at the abcl job, https://travis-ci.org/wobh/xlisp/jobs/67791084 it passes too, having run all the tests.

verdammelt commented 9 years ago

I'll review and merge your pull request today (hopefully) and then fix this problem based upon that code. I think test-exercise will "simply" need to catch any errors raised during load & running and use record-problem to register them as problems.

wobh commented 9 years ago

With #67, in I'm closing this.