search

sharplispers / clx

a fork of crhodes' fork of danb's fork of the CLX library, an X11 client for Common Lisp
Other
114 stars 46 forks source link

Test suite proposal #81

Closed uint closed 7 years ago

uint commented 7 years ago

A starting point for setting up tests for clx. Fiasco is used. The whole thing is within a separate clx/test system.

With this, test suites can be easily defined within the tests directory, probably one suite per file. An example is provided.

All defined test suites can be ran with the test runner (xlib-test:run-all-tests). The function accepts the same keywords (fiasco:run-package-tests) does. It's hooked up to asdf, so (asdf:test-system 'clx) should also work.

dkochmanski commented 7 years ago

lgtm, @PuercoPop what do you think?

PuercoPop commented 7 years ago

@dkochmanski Seems a step in the right direction

uint commented 7 years ago

From what I can tell, just using run-package-tests as provided by Fiasco would be good if we had all the tests within one test suite/package. But with enough tests, we'll probably eventually want to have a bit more hierarchy, meaning multiple test suites. When that happens, we'll have to keep manually appending new test suites to the :packages parameter of our run-package-tests call. Rather than that, I figured it'd be better to just automatically keep track of all defined test suites and have a test runner that uses that info to run them all, without hassle. Hence my definitions of run-all-tests and define-test-suite.

I probably should have made that motivation clearer from the start. Ahem.

rpgoldman commented 7 years ago

We have done something like that in our library for integrating 5AM with ASDF (to implement test-op). One thing I would suggest based on that experience: record somewhere the number of tests you expect will be run when the test suite is run, and check that the actual number of tests run agrees in the test-op.

Otherwise, if the connection between the test-op and the underlying test library goes wrong (or new tests are incorrectly wired in), then you may think you are testing when you aren't! This has definitely happened to us on occasion!

luismbo commented 7 years ago

@uint alternatively, perhaps you could send a pull request to Fiasco and augment fiasco:define-test-package to accept a parent suite. I can help you with that if needed.

uint commented 7 years ago

@rpgoldman As in, the number to check against should be input manually?

@luismbo I like the idea, but I took a look at Fiasco and I'm actually not sure it's updated much anymore. There are two PRs waiting from a year ago, without any comment.

luismbo commented 7 years ago

@uint don't let that dissuade you. We can poke @joaotavora or fork Fiasco into the the sharplispers group if needed.

rpgoldman commented 7 years ago

@uint Yes, the number to check against should be part of the defsystem. Here's an example system we use (based on 5AM):

(defsystem shop2/test
    :defsystem-depends-on ((:version "fiveam-asdf" "2"))
    :class shop-fiveam-tester
    :test-names ((pddl-tests . :shop2)
                 (protection-test . :protection-test)
                 (arity-test . :arity-test)
                 (io-tests . :arity-test)
                 (method-tests . :arity-test)
                 (umt-domain-tests . :shop2-user)
                 (blocks-tests . :shop2-user)
                 (depot-tests . :shop2-user)
                 (logistics-tests . :shop2-user)
                 (singleton-tests . :shop2-user)
                 (misc-tests . :shop2-user)
                 (minimal-subtree-tests . :shop2-user)
                 (enhanced-plan-tree . :shop2-user)
                 (theorem-prover-tests . :shop-theorem-prover-tests)
                 )
    :num-checks 422
    :depends-on ((:version "shop2" (:read-file-form "shop-version.lisp-expr")))
    :version (:read-file-form "shop-version.lisp-expr")
...
uint commented 7 years ago

@luismbo Fair enough. I'm all for working together. I'll take a look at how fiasco works when I have some time. Would you want to make it so suites can contain nested suites? Or just implement parent suites as some sort of a separate construct?

luismbo commented 7 years ago

@uint Fiasco's "test packages" are implemented on top of fiasco:defsuite which already supports nested suites, so my first hunch would be to piggyback on that.

uint commented 7 years ago

A potential solution appears.

joaotavora/fiasco#17

uint commented 7 years ago

@rpgoldman Since I'm moving away from collecting test suites on my own and using my own test runner, and toward using functionality provided by fiasco, I don't think there's as much need now for the test count check. Or is there?

rpgoldman commented 7 years ago

I'm afraid I can't tell, because I don't have experience with Fiasco. But my experience with FiveAM was that if I made a mistake, I could think that tests were being run when they weren't, hiding regressions. So I added the test count as a way to express a claim that the number of tests I saw when running manually would be the same as the number that were run using the test system when encapsulated in ASDF, and when run by people on different machines.

uint commented 7 years ago

Well, I don't think there's really a need to do that until we actually run into problems.

So if you ask me, this PR is just about ready!

dkochmanski commented 7 years ago

I have an interactive error while calling test-system:

The name "XLIB-TEST-EXAMPLE" does not designate any package.
uint commented 7 years ago

It seems quicklisp still fetches an older snapshot of fiasco. That's more than likely the cause of the problem.

CL-USER> (ql:uninstall 'fiasco)
T
CL-USER> (ql:quickload 'fiasco)
To load "fiasco":
  Load 1 ASDF system:
    alexandria
  Install 1 Quicklisp release:
    fiasco
; Fetching #<URL "http://beta.quicklisp.org/archive/fiasco/2016-10-31/fiasco-20161031-git.tgz">
dkochmanski commented 7 years ago

you mean that for this PR to work we need develop HEAD from fiasco repository?

CL-USER> (ql:uninstall :fiasco)
T
CL-USER> (ql:quickload 'fiasco)
To load "fiasco":
  Load 1 ASDF system:
    alexandria
  Install 1 Quicklisp release:
    fiasco
; Fetching #<URL "http://beta.quicklisp.org/archive/fiasco/2016-10-31/fiasco-20161031-git.tgz">
; 16.97KB
==================================================
17,378 bytes in 0.00 seconds (16970.70KB/sec)
; Loading "fiasco"
[package fiasco]..................................
[package fiasco-suites]..
(FIASCO)
CL-USER> (asdf:test-system 'clx)
; compiling file "/data/Warsztat/Repozytoria/clx/tests/package.lisp" (written 17 AUG 2017 03:59:13 PM):
; compiling (DEFPACKAGE :XLIB-TEST ...)

; /home/jack/.cache/common-lisp/sbcl-1.3.15.87-97def38-linux-x64/data/Warsztat/Repozytoria/clx/tests/package-TMP.fasl written
; compilation finished in 0:00:00.001
; compiling file "/data/Warsztat/Repozytoria/clx/tests/test.lisp" (written 17 AUG 2017 03:59:13 PM):
; compiling (IN-PACKAGE :XLIB-TEST)
; compiling (FIASCO:DEFSUITE (XLIB-ALL-TESTS :BIND-TO-PACKAGE ...))

; /home/jack/.cache/common-lisp/sbcl-1.3.15.87-97def38-linux-x64/data/Warsztat/Repozytoria/clx/tests/test-TMP.fasl written
; compilation finished in 0:00:00.031
; compiling file "/data/Warsztat/Repozytoria/clx/tests/example.lisp" (written 17 AUG 2017 03:59:13 PM):
; compiling (FIASCO:DEFINE-TEST-PACKAGE (:XLIB-TEST-EXAMPLE :IN ...))

; file: /data/Warsztat/Repozytoria/clx/tests/example.lisp
; in:
;      FIASCO:DEFINE-TEST-PACKAGE (:XLIB-TEST-EXAMPLE :IN XLIB-TEST:XLIB-ALL-TESTS)
;     (FIASCO:DEFINE-TEST-PACKAGE (:XLIB-TEST-EXAMPLE :IN XLIB-TEST:XLIB-ALL-TESTS))
; 
; caught ERROR:
;   (during macroexpansion of (FIASCO:DEFINE-TEST-PACKAGE (:XLIB-TEST-EXAMPLE :IN ...)))
;   The value
;     (:XLIB-TEST-EXAMPLE :IN XLIB-TEST:XLIB-ALL-TESTS)
;   is not of type
;     (OR (VECTOR CHARACTER) (VECTOR NIL) BASE-STRING SYMBOL CHARACTER PACKAGE)
;   when binding SB-KERNEL:PACKAGE-DESIGNATOR

; compiling (IN-PACKAGE :XLIB-TEST-EXAMPLE)
; compilation aborted after 0:00:01.124
; 
; compilation unit aborted
;   caught 3 fatal ERROR conditions
;   caught 1 ERROR condition
; Evaluation aborted on #<SB-KERNEL:SIMPLE-PACKAGE-ERROR "The name ~S does not designate any package." {100365C723}>.
uint commented 7 years ago

We need the up-to-date master branch from the fiasco repository.

joaotavora commented 7 years ago

At the risk of stating the obvious, you can git clone it into ~/quicklisp/local-projects where it will take precedence over the outdated version or wait for quicklisp to pick it up. It's pretty easy, though.

dkochmanski commented 7 years ago

Yes, I know that (thanks though). I wonder if travis does know that too. We may update travis to run tests after next QL release.

If nobody has remarks until tomorrow, I'm going to merge it as is now and bump the release to 0.7.4 as asked for in https://github.com/sharplispers/clx/issues/82 .

joaotavora commented 7 years ago

I wonder if travis does know that too.

I heavily suspect so, but have a look at fiasco's self-tests for example. They add *default-pathname-defaults* to ql:*local-project-directories* so you just need to add the git clone step to the install: portion of the travis script.

dkochmanski commented 7 years ago

merged

luismbo commented 7 years ago

Alternatively, you can have .travis.yml clone the dependency into ~/lisp as cl-travis adds that to the ASDF search path. E.g.:

install:
  - curl -L https://github.com/luismbo/cl-travis/raw/master/install.sh | sh
  - git clone https://gitlab.common-lisp.net/alexandria/alexandria.git ~/lisp/alexandria
  # ... and a bunch of other dependencies

script:
  - cl -e '(ql:quickload :cffi-tests)
           (when (cffi-tests:run-all-cffi-tests)
             (uiop:quit 1))'