Allow ordering specs by modification time #2778
Conversation
MatheusRich
force-pushed
the
order-specs-by-modification-time
branch
from
f5ad6df to
dbb5271
Compare
features/command_line/order.md
Outdated
| @@ -13,6 +13,9 @@ order of groups at each level is randomized. | |||
|
|
|||
| With `rand` you can also specify a seed. | |||
|
|
|||
| Use `modification_time` to run the most recently modified files first. You can | |||
| combine it with `--next-failure` to find the most recent failing spec. | |||
There was a problem hiding this comment.
This is a bit confusing to me in several ways.
Firstly, --next-failure says it's "Equivalent to --only-failures --fail-fast --order defined". However, the new flag is overriding the defined order.
Another thing is that the random order is also randomizing examples/groups, not just the files, while the intent of the new flag is to specify the order of the files, and not necessarily dictate to use defined or random order inside the files. I see it as complementing random/defined, not replacing them. Do you think this would be possible?
How does the new flag work with a random seed?
There was a problem hiding this comment.
I didn't understand how this could work with a random seed. Do you mean running files by modification time, but the specs inside each file in random order?
There was a problem hiding this comment.
It actually hit me by surprise that with the random order spec files are also run in random order. And I initially thought that the idea of the new flag was to change the "by load order" as in defined strategy to use mtime instead.
It looks like that those strategies are "dumb", they don't really care if it's a spec file (top-level example group) or examples from the same example group. So it's not an easy task to teach them to do one thing with specs, and another thing with examples/groups.
I suggest improving the documentation to make it clear how it all works, including the combination of the new flag with --order. I guess there's no warning that --order and the new flag (sorry for calling it like I'm a Silence of the lambs maniac, it's because we haven't settled on its name yet) are mutually exclusive right now, and they are mutually exclusive, right?.
Side observation: it's also surprising to me that if a file has several top-level example groups, random order can run them interleaved with examples from another file.
There was a problem hiding this comment.
I've updated the description. WDYT?
| @@ -13,6 +13,9 @@ order of groups at each level is randomized. | |||
|
|
|||
| With `rand` you can also specify a seed. | |||
|
|
|||
| Use `modification_time` to run the most recently modified files first. You can | |||
| combine it with `--next-failure` to find the most recent failing spec. | |||
|
|
|||
| ## Example usage | |||
|
|
|||
| The `defined` option is only necessary when you have `--order rand` stored in a | |||
There was a problem hiding this comment.
Unrelated, but this statement seems to be contradictive to the default ordering setting from the project initializer:
config.order = :random
There was a problem hiding this comment.
Thats not the default, defined is the default, but the convention overrides it, but this sentence is slightly misleading I agree.
|
Sorry for nitpicking, it's due to the feeling that in its current state this flag is not very consistent. |
MatheusRich
force-pushed
the
order-specs-by-modification-time
branch
from
ac1b312 to
42b0abf
Compare
MatheusRich
force-pushed
the
order-specs-by-modification-time
branch
from
b825e0e to
a9a3d38
Compare
|
Sorry about that, but it may take me a couple of days to get to this review. |
|
@pirj Sorry to ping you, but have you had the change to give this a second look? |
| @@ -13,6 +13,9 @@ order of groups at each level is randomized. | |||
|
|
|||
| With `rand` you can also specify a seed. | |||
|
|
|||
| Use `modification_time` to run the most recently modified files first. You can | |||
| combine it with `--next-failure` to find the most recent failing spec. | |||
|
|
|||
| ## Example usage | |||
|
|
|||
| The `defined` option is only necessary when you have `--order rand` stored in a | |||
There was a problem hiding this comment.
Thats not the default, defined is the default, but the convention overrides it, but this sentence is slightly misleading I agree.
Co-authored-by: Jon Rowe <[email protected]>
Co-authored-by: Jon Rowe <[email protected]>
|
@MatheusRich Sorry for yet another delay. There's a cleanup going on, going to get back to this PR when it's done. |
|
Thanks @MatheusRich |
…-time Allow ordering specs by modification time
…r-specs-by-modification-time Allow ordering specs by modification time --- This commit was imported from rspec/rspec-core@1efad2f.
This commit was imported from rspec/rspec-core@47b7608.
…r-specs-by-modification-time Allow ordering specs by modification time --- This commit was imported from rspec/rspec-core@4d3a9d2.
This commit was imported from rspec/rspec-core@83035e2.
Closes #2762
I'm not quite sure about the implementation, so I'll wait for your feedback