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 Properties.overrideParameters to work correctly. #463

Closed
wants to merge 2 commits into from

Conversation

non
Copy link
Contributor

@non non commented Mar 24, 2019

Currently when run from SBT (i.e. ScalaCheckFramework),
the overrideParameters method on Properties doesn't
successfully override any parameters. This was because
of the way the framework handles overrides coming from
build configuration -- the framework unconditionally
writes all parameter values, even when not explicitly
set in the build.

This commit reverses the order that these overrides are
applied, so that build overrides (which are coarser) can
be overridden by overrides in specific properties.

Fixes #221, #289, and #360.

non added 2 commits March 24, 2019 11:26
Currently when run from SBT (i.e. ScalaCheckFramework),
the overrideParameters method on Properties doesn't
successfully override any parameters. This was because
of the way the framework handles overrides coming from
build configuration -- the framework unconditionally
writes all parameter values, even when not explicitly
set in the build.

This commit reverses the order that these overrides are
applied, so that build overrides (which are coarser) can
be overridden by overrides in specific properties.
Previously we were iterating over a map, resulting in a different order than
the sequence used to build the map. In the relevant case, we can just loop
over the sequence instead.

(The map in question _is_ useful to avoid repeatedly scanning the sequence when
running certain properties by name.)
@ashawley
Copy link
Contributor

ashawley commented Apr 6, 2019

In ScalaCheckFramework.scala, a comment was added:

apply global parameters first, then allow properties to
override them. the other order does not work because
applyCmdParams will unconditionally set parameters to default
values, even when they were overridden.

It's too bad we can't have both...override parameters in a class but be able to still override them from the command-line.

That would also confuse the semantics of what an "override" does, though.

@ashawley
Copy link
Contributor

ashawley commented Apr 6, 2019

Maybe add a note that this is the case in the doc comment for Properties.overrideParameters.

Write now the doc reads:

Changes to the test parameters that are specific to this class.
Can be used to set custom parameter values for this test.

Should be something along the lines of:

Override the default test parameters for this test.

If a parameter is set in this override, it will be used over
the one set on the command-line.

@non
Copy link
Contributor Author

non commented Apr 7, 2019

@ashawley I agree the situation isn't ideal. I may try to work a bit harder to get things working as they were intended, but it's proved a tough nut to crack. Otherwise I agree that I'll need to change the documentation as you say.

@non
Copy link
Contributor Author

non commented Apr 7, 2019

All that aside, I think this order is probably better in the long run. For example, minSuccessfulTests defaults to 100. If an author overrides a specific test (A) to set that to 10 (because it's slow), and then later decides that 200 is a better baseline for their tests (via an argument in SBT), we have two options:

  • Require A to pass 200 times (very slow)
  • Still only run A 10 times (requiring the author to update A explicitly to e.g. 20)

Personally, I would consider a local override (in code) to be more specific than a global override (on the command-line), since in the other order there's no way to allow different tests to use different values.

All this aside, existing ScalaCheck docs say it should work the other way, so I'll see if I can get that working. I just thought it was worth making the case that this would possibly be a better design in the long run.

@ashawley ashawley mentioned this pull request Apr 8, 2019
18 tasks
@ashawley
Copy link
Contributor

There are other parameters besides just test counts, where you could make the same case or a different case. I tend to agree, though. If it is set in the class with overrideParameters, then it is fine to ignore even an explicit ScalaCheck argument that is given through sbt.

It seems you've made the simplest fix, and is a good first pass at making it work. It hasn't worked since ScalaCheck 1.13.0 came out a few years ago. In my eyes, fixing it like this shouldn't close other avenues for changing it later.

@non non mentioned this pull request Aug 8, 2019
non added a commit to non/scalacheck that referenced this pull request Aug 28, 2019
This commit does several things:

  1. Ensures that overrideParameters applies *after* cmdline
  2. Fixes initialSeed to work with Test.Parameters
  3. Adds parameter to disable Shrink-based shrinking
  4. Fixes numerous bugs around cmdline parsing
  5. Adds tests to ensure Test.Parameters is working
  6. Internal clean up and refactoring

This commit includes the work from typelevel#463 and subsumes it.
@non
Copy link
Contributor Author

non commented Aug 28, 2019

I ended up folding this work into #522.

@non non closed this Aug 28, 2019
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.

Properties.overrideParameters doesn't do anything?
2 participants