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

Runtime stat with test names containing colons symbol #574

Merged
merged 1 commit into from Jul 15, 2017

Conversation

ineverov
Copy link
Contributor

@ineverov ineverov commented Jul 11, 2017

In case of example containing : like some_spec.rb[1:2:3] or 'some:spec.rb' runtimes file will contain records like

some_spec.rb[1:2:3]:123
some:spec.rb:321

and parsed file will have wrong name/runtime statistic.

@grosser
Copy link
Owner

grosser commented Jul 11, 2017

that should never happen ... runtime log is supposed to only include file names ... so better fix the logger that produced this and not keep the bad format ...

@ineverov
Copy link
Contributor Author

ineverov commented Jul 11, 2017

@grosser thanks for fast response

The problem I'm trying to solve right now is:
I'd like to store full RSpec example id in runtime statistic, which will allow running set of examples instead of files.

It would be great if parallel_test will support such format and treat /<filename>:<runtime>$/ as filename runtime. Currently, if for some reason filename will contain colon, like filename_s:pec.rb:123 it will parse it as filename: 'filename_s', runtime: 'pec.rb:123'

@grosser
Copy link
Owner

grosser commented Jul 11, 2017

if you already do your own reporter, how about patching this method to do your bidding too ?
... feel kinda awkward adding code that most users will never need ...

@ineverov
Copy link
Contributor Author

I understand you as the owner of the gem which is very popular and I really love it.
But here are few things why I created this PR:

  1. There is problem with running tests with runtimes where filenames has colon(s) some:test.rb
  2. Grouping tests by example are more accurate then by file. Example, 10 tests each 1 second in 2 files (with 1 example and 9 examples); with file grouping, one process will take 1 sec, other 9. With example grouping, they both will take 5 seconds.
  3. Personally, for me, patching method which is part of gem (which will require consistent updates with new cool features added and bug fixes) is not the same as adding custom RSpec formatter
  4. If you ever decide to add feature allowing to run set of custom tasks in parallel with runtimes, this will again be an issue if task contains colon

@grosser
Copy link
Owner

grosser commented Jul 11, 2017

atm the times are not added so having the examples being parsed would be wrong ?
a.rb[1:2]:1.2 + a.rb[1:3]:1.3 would end up as a.rb with 1.3s

@ineverov
Copy link
Contributor Author

ineverov commented Jul 11, 2017

This PR is more about fixing the issue with a colon in a filename (which will make my life easier and hope someone else too).

Here is description what I'm doing in my case:
I have 2 formatters:

  1. just output list of examples in format filename.rb[1:2:3] and I run rspec with --dry-run. So it really fast generate list of examples like this (my.log)
a.rb[1:1]:1
a.rb[1:2]:1
  1. time formatter - stores runtimes in format filename.rb[1:2:3]:1.2

What I do is 2 steps:

rspec --format ListFormatter --out list.txt a.rb
parallel_tests -t rspec -n 2 --group-by runtime $(cat list.txt) --runtime-log my.log

Which is basically like this and finishes in about 1 sec

parallel_tests -t rspec -n 2 --group-by runtime --runtime-log my.log  a.rb[1:1] a.rb[1:2] 

@ineverov
Copy link
Contributor Author

ineverov commented Jul 11, 2017

In example you provided above you are right, they are not added if you pass filename to parallel_tests. But if you pass examples with ids it will end up 1.3 instead of 2.5 for whole file

@ineverov
Copy link
Contributor Author

@grosser btw, it can help with this one #252 for rerunning only failing examples instead of failing files

@grosser
Copy link
Owner

grosser commented Jul 15, 2017

  • this looks like it would break if a single line gets moved in a file ?
  • won't this stop working once there are >1k examples since it will be too many arguments ?

@grosser
Copy link
Owner

grosser commented Jul 15, 2017

looks like this could be useful for some weird cases ... and it's pretty cheap ... so I'll just merge ... but still not feeling very convinced this is a good idea ...

@grosser grosser merged commit 658d46d into grosser:master Jul 15, 2017
@ineverov
Copy link
Contributor Author

@grosser thanks for merging it

  1. nope, if you use scenario ids instead of line numbers. But yet this will break if you re-arrange scenarios in file
  2. I'm not sure for all OS, but on Mac it supports 262144 arguments
ineverov$ getconf ARG_MAX
262144

@ineverov ineverov deleted the colon branch July 18, 2017 22:04
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

3 participants