Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create run-parachute script #10

Merged
merged 6 commits into from Sep 3, 2019

Conversation

@neil-lindquist
Copy link
Contributor

commented Sep 2, 2019

Similar to run-fiveam and run-prove, the script is designed to make it easy to run parachute tests on CI platforms (in fact, it's run-fiveam with fiveam replaced with parachute).

If the script is installed by Roswell, the foo-test package in the foo/test system can be tested with
run-parachute-l foo/test-parachute foo-tests-parachute (or roswell/run-parachute.ros -e t -l foo/test-parachute foo-tests-parachute if measuring coverage and excluding the t directory from coverage)

Example of tests passing: https://travis-ci.org/neil-lindquist/cl-ci-test/builds/579901976
Example of tests failing: https://travis-ci.org/neil-lindquist/cl-ci-test/builds/579896736

Create run-parachute script
Like run-fiveam and run-prove, the script is designed to make it easy to
run parachute tests on CI platforms.
|#

;cmucl crashes with silent on
(ql:quickload '(:ci-utils :ci-utils/coveralls :parachute :iterate)

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

Is all this stuff necessary? Seems to just be pointlessly polluting the test environment.

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

ci-utils/coveralls wraps cl-coveralls when the user requests coverage data be sent to Coveralls, and avoids loading all of cl-coveralls dependencies if not needed (there are 8 first order dependencies, with some second and third order dependencies).
ci-utils itself isn't directly needed (although loaded by ci-utils anyways), so I can remove that quickload.
For Iterate, I can refactor away the dependency, it just makes the code simpler when taking a variable number of arguments each iteration.

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

I'm a bit confused why coveralls is needed at all. Shouldn't coverage be determined when someone works on tests, rather than every time CI runs for whatever reason?

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

Any time when branches in the execution paths are added or refactored, it's good to double check that there isn't a case that isn't tested. I've always see coveralls used with reports from every CI run in order to watch for degradation in code coverage more closely. So, I wanted to make sure that style of CI is supported.

This script only measures coverage when the COVERALLS environmental variable is set, so the overhead is limited for workflows that don't want continuous coverage checks.

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

I see, that's fair enough.

(ci-utils/coveralls:with-coveralls excluded
(when loaded-systems
(ql:quickload loaded-systems))
(parachute:test-toplevel (mapcar 'read-from-string tests))))))

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

The file does not fit with the rest of the style rules employed in the library.

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

I'm not sure what style issues in particular I should adjust. Is there a written style guide that I should look at, or specific changes you want?

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

I don't have a guide written, so here's what I noticed:

  • Constants like NIL, T, etc need to be upper-case.
  • Comments need the proper number of semicolons: one semicolon for same-line, two for in-line, three for top-level, four for sectioning.
  • Symbols in package definitions and usage need to be uninterned (#:foo).
  • Numerical inequalities always use < or <=.
  • The first clause in a cond is on the same line as the cond.
  • Don't use nconc unless the performance benefit is proven.

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

Ok, I'll apply those changes.

((or (string= "--quickload" arg) (string= "-l" arg))
(collect (next arg-list) into loaded-systems))
((or (string= "--exclude" arg) (string= "-e" arg))
(collect (next arg-list) into excluded))

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

Would be good to allow choosing the report type to use during testing.

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

Good call. I'll work on adding that.

#!/bin/sh
#|-*- mode:lisp -*-|#
#|
exec ros -Q -- $0 "$@"

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

Not everyone uses roswell. Testing a few implementations if roswell is not available would be good.

This comment has been minimized.

Copy link
@neil-lindquist

neil-lindquist Sep 2, 2019

Author Contributor

Hmm. Because it's set up using the conventions for .ros scripts, I'm having issues figuring out how to work around the combination of shebang and main function. Looking at sbcl's command line options specifically, the ability to ignore the shebang line and the ability to run additional code after the script.

The best solution I'm coming up with for supporting non-Roswell systems is to bundle a second copy of the script that exists as a plain lisp source file. The code duplication is necessary for this approach because Roswell makes copies of script files it knows about, so referencing "local" files gets problematic.

There's also a slight inconvenience of needing to find the path when not using Roswell, since Roswell places all of the ros scripts of installed through its quicklisp in a specific directory. Although, it could be downloaded with curl or a similar tool right to a convenient directory by the user.

This comment has been minimized.

Copy link
@Shinmera

Shinmera Sep 2, 2019

Owner

You could feed the remainder of the file (cutting off the first line) through stdin on quite a few of them I think.

@neil-lindquist

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I've made changes for all of your suggestions, except providing support for non-Roswell implementations, I'm still looking at that. The changes are spread over a bunch of commits so that it was easier to incrementally rebase. I'll rebase them to a single commit once this is otherwise ready.

The loop for parsing the arguments is kind of hard to read at the moment, since the loops in parachute aren't indented to match if-else type structures. I might try replacing it with let and a basic while loop.

Refactor the argument parsing loop to avoid using collect
It allows the loop to be written more readably
@neil-lindquist

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I refactored the loop, and it's more readable now that it's not using loop's control flow.

@Shinmera

This comment has been minimized.

Copy link
Owner

commented Sep 3, 2019

Looks good now, thanks a lot!

I'll accept further patches if you can figure out a way to support non-roswell invocations.

@Shinmera Shinmera merged commit 5f9a19c into Shinmera:master Sep 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.