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

filter properties based on a criteria #267

Merged
merged 6 commits into from
Oct 24, 2016

Conversation

ssanj
Copy link
Contributor

@ssanj ssanj commented Sep 25, 2016

Allows properties to be filtered based on a supplied criteria. This
allows running of a subset of property tests for a given specification.
The test criteria is specified by the '-f' option.

Eg:

Given the following properties for a String specification:

  1. StringProps.String map
  2. StringProps.String reverse

when supplied with a "map" criteria, Scalacheck will only run the map property:

  1. StringProps.String map

On the commandline:

scala -cp scalacheck.jar:. StringProps -f "map"

Through SBT:

test-only *StringProps -- -f map

The primary motivation for creating this functionality was so that
I could selectively run a subset of properties that were expensive
to run every time. It also helps focus on the current property being
tested without having to run all properties of a specification each
time.

I'm not sure I have covered all the bases here - I have tested this
through SBT and the commandline, but I wonder if there are any other
scenarios I should test this through.

I'm happy to get some feedback on this and improve it as necessary.

@non
Copy link
Contributor

non commented Sep 25, 2016

Hi @ssanj,

If you're going to do this filtering by matching on test/property names, I think the -f argument should accept a regular expression. That means instead of "map" you'd use something like ".* map" instead. Right now, the fact that you can only match against the property name (and not the test name as well) seems like an unnecessary limitation.

What do you think?

@ssanj
Copy link
Contributor Author

ssanj commented Sep 25, 2016

Hi @non,

Thanks for your feedback.

  1. Matching on regular expression - Good call. I'm not sure why I didn't do this in the first place. It's probably because I based this functionality on similar functionality in ScalaTest (-z option) which uses a substring as opposed to a regular expression.
  2. Matching against the test name as well as property name - I'm happy to change this to include the test name. The reason I didn't was because, I primarily use this feature from SBT and as such, SBT's test-only task filters the test names for you:

test-only <test name filter> -- -f <property name filter>

We could pass the full match for the test name as well to -f, but that would only apply to what SBT hands over to ScalaCheck after filtering on test name. I thought this might be confusing to use as sometimes based on your SBT test name filter, your property name would not match any tests (because SBT has already filtered it out)

test-only *StringProp -- --f "List.* map"

When running Scalacheck from the command line, we only specify a single test (currently - to mu knowledge) so I assumed there was no need to filter by test name. I'm not sure if there's a way to specify a wildcard for test name here?

scala -cp scalacheck.jar:. StringProps

So if we match on test name as well and run the filter from the command line, would we have to enumerate all the tests for the current project and then apply the test and property name filter to those found? I'm not sure what we would specify as the test name in this case?

scala -cp scalacheck.jar:. <what would we specify here?> -f "String.* map"

One option would be to make the test name argument optional when specified with the -f option. Or we could use a wildcard:

scala -cp scalacheck.jar:. * -f "String.* map"

What are your thoughts on this?

@non
Copy link
Contributor

non commented Sep 25, 2016

@ssanj Ah, I hadn't thought about the fact that test-only is already allowing you to select which test(s) you want. Given that, I think using a regex to match just the property name itself is fine. The nice thing about that is that it means people can use a suffix (or prefix) to "group" their properties and then run those groups using a regex.

@ssanj
Copy link
Contributor Author

ssanj commented Sep 26, 2016

@non cool. Sounds good.

I noticed that Travis had failed with some binary compatibility issues:

[info] scalacheck: found 5 potential binary incompatibilities while checking against org.scalacheck:scalacheck_2.11:1.13.1 (filtered 2)

[error] * method copy(Int,Int,Int,Int,org.scalacheck.Test#TestCallback,Float,scala.Option)org.scalacheck.Test#Parameters#cp in class org.scalacheck.Test#Parameters#cp does not have a correspondent in current version

[error] filter with: ProblemFilters.excludeDirectMissingMethodProblem

[error] * method this(org.scalacheck.Test#Parameters,Int,Int,Int,Int,org.scalacheck.Test#TestCallback,Float,scala.Option)Unit in class org.scalacheck.Test#Parameters#cp does not have a correspondent in current version

[error] filter with: ProblemFilters.excludeDirectMissingMethodProblem

[error] * the type hierarchy of object org.scalacheck.Test#Parameters#cp is different in current version. Missing types {scala.runtime.AbstractFunction7}

[error] filter with: ProblemFilters.excludeMissingTypesProblem

[error] * method apply(Int,Int,Int,Int,org.scalacheck.Test#TestCallback,Float,scala.Option)org.scalacheck.Test#Parameters#cp in object org.scalacheck.Test#Parameters#cp does not have a correspondent in current version

[error] filter with: ProblemFilters.excludeDirectMissingMethodProblem

[error] * abstract method runFilter()scala.Option in class org.scalacheck.Test#Parameters is present only in current version

[error] filter with: ProblemFilters.excludeReversedMissingMethodProblem

java.lang.RuntimeException: scalacheck: Binary compatibility check failed!`

As I've added a new parameter to Test#Parameters and updated cp in the process, should I add this PR against another version branch?

@non
Copy link
Contributor

non commented Sep 26, 2016

@ssanj As far as MiMA goes let's wait and see what @rickynils thinks.

@rickynils
Copy link
Contributor

This looks useful! I think we should simply do 1.13.3 release, and then bump the master branch for 1.14 development. Then we can merge this and #263. I usually maintain a 1.13.x branch with back-ported stuff if needed, rather than creating a new branch for the next release.


# ide
*.ctags_srcs/
.tags
Copy link
Contributor

Choose a reason for hiding this comment

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

@ssanj Maybe those gitignore changes could be moved to another commit/PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rickynils sure.

1. Added filtering tests by regular expression
2. Removed .gitignore entries as per suggestion by @rickynils
Renamed runFilter => propFilter as it makes the intent clearer - filter
properties on some predicate.

Moved regular expression creation to outside of loops to reduce
performance overhead.
Moved calculation of params out of loop to increase performance.
An initial assumption was that there was a 1:1 mapping between
test class names and properties name. This is not true as multiple
test classes can define the same properties.

Eg:

case PropObject extends Properties("TestSpecification") {
  def property1 = ...
  def property2 = ...
  def property3 = ...

}
case PropClass  extends Properties("TestSpecification") {
  def property4 = ...
}

case OtherProp extends Properties("OtherSpecification") {
  def property3 = ...
}

This makes it hard to run all "TestSpecification" properties through
SBT:

test-only *Prop* //runs all tests in PropObject, PropClass and OtherProp

This would result in output of the form:

TestSpecification.property1 //from PropObject
TestSpecification.property2 //from PropObject
TestSpecification.property3 //from PropObject
TestSpecification.property4 //from PropClass
OtherSpecification.property3 //from OtherProp

What if we want to only run property3 in TestSpecification?

test-only *Prop* -- -f "property3"

The above would result in output of the form:

TestSpecification.property3 //from PropObject
OtherSpecification.property3 //from OtherProp

We don't want the OtherSpecification property included, but we have
no way of filtering it out.

As such it would be useful to allow filtering on the full property name
which would include the grouping properties name as well as the property
under test. This allows us to do:

test-only *Prop* -- -f "TestSpecification\\.property3"

This gives us the correct output:

TestSpecification.property3 //from PropObject
@ssanj ssanj force-pushed the pr_test_run_filter branch from 02bde45 to af16e6c Compare September 28, 2016 14:43
@ssanj
Copy link
Contributor Author

ssanj commented Sep 28, 2016

@non I've added in support for filtering on the Properties name as well on the property name as you requested before. I found some examples in ScalaCheck that use different test classes/specifications to define properties across the same Properties name. In those cases, SBT only filters on the class/specification name, which is different to the Properties name.

I've added some thoughts on why in the comments to 3d5d030.

@ssanj
Copy link
Contributor Author

ssanj commented Sep 29, 2016

Seems like Travis is having issues downloading dependencies:

:: problems summary ::

:::: WARNINGS

    [FAILED     ] org.scala-sbt#main;0.13.9!main.jar: The HTTP response code for https://repo.typesafe.com/typesafe/ivy-releases/> org.scala-sbt/main/0.13.9/jars/main.jar did not indicate a success. See log for more detail. (345ms)

    [FAILED     ] org.scala-sbt#main;0.13.9!main.jar: The HTTP response code for https://repo.typesafe.com/typesafe/ivy-releases/> org.scala-sbt/main/0.13.9/jars/main.jar did not indicate a success. See log for more detail. (345ms)

==== typesafe-ivy-releases: tried

  https://repo.typesafe.com/typesafe/ivy-releases/org.scala-sbt/main/0.13.9/jars/main.jar

    [FAILED     ] org.scala-sbt#compiler-interface;0.13.9!compiler-interface-bin.jar: The HTTP response code for https://> repo.typesafe.com/typesafe/ivy-releases/org.scala-sbt/compiler-interface/0.13.9/jars/compiler-interface-bin.jar did not > indicate a success. See log for more detail. (362ms)

    [FAILED     ] org.scala-sbt#compiler-interface;0.13.9!compiler-interface-bin.jar: The HTTP response code for https://> repo.typesafe.com/typesafe/ivy-releases/org.scala-sbt/compiler-interface/0.13.9/jars/compiler-interface-bin.jar did not > indicate a success. See log for more detail. (362ms)

==== typesafe-ivy-releases: tried

  https://repo.typesafe.com/typesafe/ivy-releases/org.scala-sbt/compiler-interface/0.13.9/jars/compiler-interface-bin.jar

    ::::::::::::::::::::::::::::::::::::::::::::::

    ::              FAILED DOWNLOADS            ::

    :: ^ see resolution messages for details  ^ ::

    ::::::::::::::::::::::::::::::::::::::::::::::

    :: org.scala-sbt#main;0.13.9!main.jar

    :: org.scala-sbt#compiler-interface;0.13.9!compiler-interface-bin.jar

    ::::::::::::::::::::::::::::::::::::::::::::::

:: USE VERBOSE OR DEBUG MESSAGE LEVEL FOR MORE DETAILS

download failed: org.scala-sbt#main;0.13.9!main.jar

download failed: org.scala-sbt#compiler-interface;0.13.9!compiler-interface-bin.jar

Error during sbt execution: Error retrieving required libraries

(see /home/travis/.sbt/boot/update.log for complete log)

Error: Could not retrieve sbt 0.13.9>

@non
Copy link
Contributor

non commented Oct 16, 2016

@ssanj I think you can re-run Travis by closing and reopening (or @rickynils can re-run it directly).

@ssanj
Copy link
Contributor Author

ssanj commented Oct 16, 2016

Closing (and will then reopen) to rerun Travis.

@ssanj ssanj closed this Oct 16, 2016
@ssanj ssanj reopened this Oct 16, 2016
@ssanj
Copy link
Contributor Author

ssanj commented Oct 16, 2016

@non The Binary compatibility checks still fail as I have changed the API. The other tests have passed.

@non
Copy link
Contributor

non commented Oct 17, 2016

Looks good, thanks!

@rickynils rickynils merged commit 7c3dee3 into typelevel:master Oct 24, 2016
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.

3 participants