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

Allow users to filter tests using command-line arguments #44

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

cfinegan
Copy link

This pull request changes the behavior of the define-check macro so that it examines the current-command-line-arguments, which it interprets as a set of names, files, and line numbers to run. More specific information about the filtering behavior is available in the docs under "Filtering Tests with Command-line arguments."

@jackfirth jackfirth self-requested a review June 20, 2017 23:20
(define new-files (cons (regexp (substring arg 5)) files))
(values names new-files lines)]
[(regexp-match-exact? #rx"[Ll][Ii][Nn][Ee]:.+" arg)
(define new-lines (cons (string->number (substring arg 5)) lines))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if not numberable string

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this so it ignore invalid line numbers.

@jeapostrophe
Copy link
Contributor

@jackfirth 's recent changes to the data structures broke your in-development version, so please update your fork and fix it up. See the Travis CI log for the problem.

(check-equal? 1 2))
(check-equal? 2 3))]

In the above example, the former test runs because its @racket['name] field
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do test-case and test-suite set the name field?

(So, if I have (test-case "foo" ....) will raco test foo file.rkt run only that test case?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops just realized I need to write raco test ++arg foo file.rkt.

It would be good to have an example of raco test .... in these docs.

['location loc])
(check-equal? 1 2)))]

The above example is not run because, even though the test name and file number
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file number

line number

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed this.

@rmculpepper
Copy link
Contributor

I don't think this feature should be built into rackunit. If you want to conditionally execute parts of your test suite, say so with when and unless and perhaps a DSL built on top of those features. Here's a sketch of one possible form:

(filtering-begin
  #:name "name"
  #:tags '(...)
  #:when ...expression...
  body ...)

The filtering-begin macro could use the name, tags, when-condition, etc (together with information from the command-line arguments or environment variables) to decide whether to execute its body. You could even have it wrap each body expression in something that checked the line number.


Some other comments:

The granularity for selective execution should be test cases, not checks. A check might guard another check or an action that shouldn't be executed if the first check fails. In contrast, test cases are supposed to be independent.

A library should probably not be parsing command-line arguments, and especially not with such generic names. An environment variable is more idiomatic for adjusting library behavior.

In any case, the command-line arguments (or environment variable) should not be re-parsed/processed each time a check executes. Instead, the library should define another parameter (current-execution-filter?) and initialize that parameter once when the library is loaded.

If this must be tied into rackunit, it should be implemented via the existing current-test-case-around parameter. Add a library (rackunit/filter-execution?) that defines and initializes current-execution-filter, modifies current-test-case-around to consult it, etc.

@samth
Copy link
Sponsor Member

samth commented Jun 22, 2017

I disagree with @rmculpepper here. Handling selective execution is a big part of making a nice-to-use testing system, and command line parsing is a very effective way of doing this.

My experience with py.test has been really positive. This feature is built in there, and you just provide a simple command line argument to specify it.

@jeapostrophe
Copy link
Contributor

My thoughts, @rmculpepper

Re: check vs test-case --- I think that most Rackunit users don't use test-case and just put a bunch of check-equal?, etc in their test submodule.

Re: parsing & reparsing --- I think your idea is fine. Personally, I would implement it with a global variable and a cache, rather than a parameter. @cfinegan can you do @rmculpepper 's idea?

Re: command line vs env variable --- I think that they are orthogonal and I was looking for something with good mouth feel. My preference was "raco test file --- name" but @mflatt objected so we have "raco test ++arg name file" which feels better to the taste than "PLT_RACKUNIT_FILTER=name raco test file"

Are you blocking the change or are you convinced in any way?

@rmculpepper
Copy link
Contributor

I'm unconvinced. I'll write a more detailed response later tonight.

@rmculpepper
Copy link
Contributor

I have two main problems with the current design and implementation.

  1. it doesn't avoid much execution (not useful)
  2. it breaks tests that already use command-line arguments (like mine)

First, because the decision of whether to execute or not happens within individual checks and after the evaluation of their arguments, very little evaluation is actually avoided. Suppose we have the following tests, and suppose the we have indicated that we want to run only the first one:

(check-equal? (expensive-computation 1 2) 3)
(check-equal? (expensive-computation 3 4) 7)
(test-case "resource test"
  (define r (create-resource))
  (check-equal? (expensive-computation 3 4)
                (access-resource 'a)))

The second expensive computation still gets run. The resource r still gets created and accessed, and the third expensive computation still gets run. If that's the intended behavior, then it would be much better to just implement a filter on the reporting sent to stderr. Otherwise, the right granularity for selective execution is the test case, not the check.

Second, the use of command-line arguments breaks tests that already use command-line arguments, such as the db tests. I introduced an error into the db tests and ran racket -l tests/db/all-tests sl pg (that is, execute the tests with sqlite3 and postgresql drivers). I got back "138 successes, 0 failures ... 98 successes, 0 failures..." because the presence of command-line arguments caused rackunit to skip all of the checks inside the test cases.

My recommendation is to build a completely separate filtered execution macro; I don't think it belongs in the rackunit core. (But possibly as a rackunit library.) Wrapping the form around test cases or top-level checks or sequences thereof solves the first problem, and making it opt-in (as opposed to changing the meaning of existing rackunit programs) helps with the second.

@jackfirth
Copy link
Sponsor Collaborator

jackfirth commented Jun 23, 2017

I agree with @rmculpepper. Having said that, @samth could you open an issue describing what sort of test filtering you'd like for the Typed Racket tests (I assume TR is your motivation here?)

I think if we had a concrete use case it'd be much easier to work out the right API for this.

@mfelleisen
Copy link

mfelleisen commented Jun 23, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants