Add support to profdata file format #92

Merged
merged 73 commits into from Jan 19, 2016

Conversation

Projects
None yet
@viteinfinite
Contributor

viteinfinite commented Jun 15, 2015

Hey,
I'm adding experimental support to the profdata file format (i.e. the format used in Xcode 7 beta).

The current implementation calls llvm-cov and reads the subsequent output.
Two new configuration parameters have been added:

  • scheme: allows the selection of a scheme; it is used to find the folder containing the correct instrumentation data
  • input-format: in case the value is profdata, Slather will look for the Coverage.profdata file inside the build directory

Branch information is not included at the moment as I still have to figure out how (and if) this information is actually included in the Coverage.profdata file.

Please feel free to discuss or inquiry about any implementation detail.

@sync

This comment has been minimized.

Show comment
Hide comment
@sync

sync Jun 18, 2015

  • also see viteinfinite#1 (minor fix)
  • usage: slather coverage --input-format profdata --scheme YauShop YauShop.xcodeproj
  • output extract with Xcode 7 coverage:
YauShop/Utils/TegQ.swift: 4 of 10 lines (40.00%)
YauShop/Utils/TegRecycler.swift: 0 of 11 lines (0.00%)
YauShop/Utils/TegRegex.swift: 12 of 15 lines (80.00%)
YauShop/Utils/TegScreenSize.swift: 0 of 22 lines (0.00%)
YauShop/Utils/TegScreenType.swift: 0 of 3 lines (0.00%)
YauShop/Utils/TegSecureRandom.swift: 11 of 15 lines (73.33%)
YauShop/Utils/TegString.swift: 27 of 27 lines (100.00%)
YauShop/Utils/TegTextFileLoader.swift: 4 of 4 lines (100.00%)
YauShop/Utils/TegTickTock.swift: 0 of 23 lines (0.00%)
YauShop/Utils/TegUIColor.swift: 37 of 37 lines (100.00%)
YauShop/Utils/TegUrlEncoder.swift: 5 of 5 lines (100.00%)
YauShop/Utils/TegViewPosition.swift: 0 of 25 lines (0.00%)
YauShop/Utils/UI/KeyboardObserver.swift: 31 of 34 lines (91.18%)
YauShop/Utils/UI/RoundedButton.swift: 7 of 10 lines (70.00%)
YauShop/Utils/UI/RoundedView.swift: 10 of 10 lines (100.00%)
Test Coverage: 39.79%
Slathered

sync commented Jun 18, 2015

  • also see viteinfinite#1 (minor fix)
  • usage: slather coverage --input-format profdata --scheme YauShop YauShop.xcodeproj
  • output extract with Xcode 7 coverage:
YauShop/Utils/TegQ.swift: 4 of 10 lines (40.00%)
YauShop/Utils/TegRecycler.swift: 0 of 11 lines (0.00%)
YauShop/Utils/TegRegex.swift: 12 of 15 lines (80.00%)
YauShop/Utils/TegScreenSize.swift: 0 of 22 lines (0.00%)
YauShop/Utils/TegScreenType.swift: 0 of 3 lines (0.00%)
YauShop/Utils/TegSecureRandom.swift: 11 of 15 lines (73.33%)
YauShop/Utils/TegString.swift: 27 of 27 lines (100.00%)
YauShop/Utils/TegTextFileLoader.swift: 4 of 4 lines (100.00%)
YauShop/Utils/TegTickTock.swift: 0 of 23 lines (0.00%)
YauShop/Utils/TegUIColor.swift: 37 of 37 lines (100.00%)
YauShop/Utils/TegUrlEncoder.swift: 5 of 5 lines (100.00%)
YauShop/Utils/TegViewPosition.swift: 0 of 25 lines (0.00%)
YauShop/Utils/UI/KeyboardObserver.swift: 31 of 34 lines (91.18%)
YauShop/Utils/UI/RoundedButton.swift: 7 of 10 lines (70.00%)
YauShop/Utils/UI/RoundedView.swift: 10 of 10 lines (100.00%)
Test Coverage: 39.79%
Slathered
@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jun 18, 2015

Contributor

@sync thank you for the usage sample!

Contributor

viteinfinite commented Jun 18, 2015

@sync thank you for the usage sample!

@sync

This comment has been minimized.

Show comment
Hide comment
@sync

sync Jun 18, 2015

right now it seems that:

  • the JSON exporter doesn't work
  • coverall doesn't work
  • cobertura is working

sync commented Jun 18, 2015

right now it seems that:

  • the JSON exporter doesn't work
  • coverall doesn't work
  • cobertura is working
@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jun 21, 2015

Contributor

Thank you for the feedback, I've been working on some improvements. I'll push the modifications in some hours.

Contributor

viteinfinite commented Jun 21, 2015

Thank you for the feedback, I've been working on some improvements. I'll push the modifications in some hours.

viteinfinite added some commits Jun 21, 2015

Merge branch 'feature-temp-profdata' of https://github.com/viteinfini…
…te/slather into feature-temp-profdata

* 'feature-temp-profdata' of https://github.com/viteinfinite/slather:
  Remove useless file
  Fix tests
  Remove useless files
  Fix unit tests
  Add more precise path handling
  Add scheme and ignore
  Export coverage base info into modules
  Fix coveralls_spec
  Minor fixes in spec_helper
  Re-add coveralls support
  Re-add coveralls support
  Add tests
  [WIP] Refactor interface of coverage_file
  [WIP] Add unit tests
  [WIP] Add experimental support to swift
@marklarr

This comment has been minimized.

Show comment
Hide comment
@marklarr

marklarr Jun 22, 2015

Contributor

This is cool -- I'll check it out this week!

Contributor

marklarr commented Jun 22, 2015

This is cool -- I'll check it out this week!

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Aug 5, 2015

Contributor

@marklarr Any update on this?

Contributor

viteinfinite commented Aug 5, 2015

@marklarr Any update on this?

petester42 and others added some commits Aug 8, 2015

Merge branch 'petester42-master' into feature-profdata
* petester42-master:
  Add spec for framework coverage
  Fix llvm-cov path and add framework coverage

@viteinfinite viteinfinite referenced this pull request in viteinfinite/slather Aug 10, 2015

Closed

fix llvm-cov path and add framework coverage #4

@mjdetullio

This comment has been minimized.

Show comment
Hide comment
@mjdetullio

mjdetullio Aug 17, 2015

Built and installed the Gem from source. Using the Cobertura reporting now and recording it in Jenkins. Works great.

/usr/local/bin/slather \
        coverage \
        --cobertura-xml \
        --output-directory "${WORKSPACE}/build/coverage-reports" \
        --input-format profdata \
        --build-directory "${WORKSPACE}/DerivedData" \
        --source-directory . \
        --ignore '../**' \
        --ignore 'SomeSource/generated/**' \
        --ignore 'Tests/**' \
        --scheme "MyTests" \
        MyProject.xcodeproj

One thing I noticed is it has to be run from the source root, otherwise the rest of the tree is displayed on the report, regardless of the --source-directory option. Just ended up doing a pushd/popd into the source root and setting the source dir as the current dir.

Built and installed the Gem from source. Using the Cobertura reporting now and recording it in Jenkins. Works great.

/usr/local/bin/slather \
        coverage \
        --cobertura-xml \
        --output-directory "${WORKSPACE}/build/coverage-reports" \
        --input-format profdata \
        --build-directory "${WORKSPACE}/DerivedData" \
        --source-directory . \
        --ignore '../**' \
        --ignore 'SomeSource/generated/**' \
        --ignore 'Tests/**' \
        --scheme "MyTests" \
        MyProject.xcodeproj

One thing I noticed is it has to be run from the source root, otherwise the rest of the tree is displayed on the report, regardless of the --source-directory option. Just ended up doing a pushd/popd into the source root and setting the source dir as the current dir.

@marklarr

This comment has been minimized.

Show comment
Hide comment
@marklarr

marklarr Aug 18, 2015

Contributor

Hey @viteinfinite -- I was out on vacation for a couple weeks. I'll get to looking at this later this week!

Contributor

marklarr commented Aug 18, 2015

Hey @viteinfinite -- I was out on vacation for a couple weeks. I'll get to looking at this later this week!

@marklarr

This comment has been minimized.

Show comment
Hide comment
@marklarr

marklarr Aug 21, 2015

Contributor

@viteinfinite Is this ready to be merged? It looks good to me. We just need to resolve the conflicts.

Contributor

marklarr commented Aug 21, 2015

@viteinfinite Is this ready to be merged? It looks good to me. We just need to resolve the conflicts.

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Aug 21, 2015

Contributor

@marklarr I can close this PR and submit a new one resolving the conflicts or you manually merge them. Either way, the work on the feature itself is completed.

Contributor

viteinfinite commented Aug 21, 2015

@marklarr I can close this PR and submit a new one resolving the conflicts or you manually merge them. Either way, the work on the feature itself is completed.

viteinfinite and others added some commits Jun 13, 2015

Merge branch 'feature-temp-profdata' of https://github.com/viteinfini…
…te/slather into feature-temp-profdata

* 'feature-temp-profdata' of https://github.com/viteinfinite/slather:
  Remove useless file
  Fix tests
  Remove useless files
  Fix unit tests
  Add more precise path handling
  Add scheme and ignore
  Export coverage base info into modules
  Fix coveralls_spec
  Minor fixes in spec_helper
  Re-add coveralls support
  Re-add coveralls support
  Add tests
  [WIP] Refactor interface of coverage_file
  [WIP] Add unit tests
  [WIP] Add experimental support to swift

@mattdelves mattdelves referenced this pull request Aug 29, 2015

Closed

Feature profdata #99

lib/slather/profdata_coverage_file.rb
+ end
+
+ def supported_file_extensions
+ ["swift"]

This comment has been minimized.

@mgrebenets

mgrebenets Sep 5, 2015

Does this limit the tool to extract coverage data for Swift files only?

@mgrebenets

mgrebenets Sep 5, 2015

Does this limit the tool to extract coverage data for Swift files only?

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Sep 9, 2015

Contributor

@mgrebenets Yep, that needs to be changed.

Contributor

viteinfinite commented Sep 9, 2015

@mgrebenets Yep, that needs to be changed.

@mgrebenets

This comment has been minimized.

Show comment
Hide comment
@mgrebenets

mgrebenets Sep 9, 2015

@viteinfinite I actually have built the gem from the branch and used it like this on Objective-C only project.

slather coverage --input-format profdata -x --ignore "../**/*/Xcode*" --output-directory test-report MyProject.xcodeproject

And it got all the coverage data for .m files.

Btw, it would be really nice to be able to specify custom path to build directory and (maybe) derived data directory. Some, including my company, use build scripts with custom CONFIGURATION_BUILD_DIR, but slather expects all the things to sit in default locations. So I can't use the tool just for picking up and converting profdata to, say, cobertura, unless I used standard build scripts.

@viteinfinite I actually have built the gem from the branch and used it like this on Objective-C only project.

slather coverage --input-format profdata -x --ignore "../**/*/Xcode*" --output-directory test-report MyProject.xcodeproject

And it got all the coverage data for .m files.

Btw, it would be really nice to be able to specify custom path to build directory and (maybe) derived data directory. Some, including my company, use build scripts with custom CONFIGURATION_BUILD_DIR, but slather expects all the things to sit in default locations. So I can't use the tool just for picking up and converting profdata to, say, cobertura, unless I used standard build scripts.

+ class ProfdataCoverageFile
+
+ include CoverageInfo
+ include CoverallsCoverage

This comment has been minimized.

@eliperkins

eliperkins Sep 9, 2015

Member

Is there somewhere that we are explicitly needing Coveralls in this class?

@eliperkins

eliperkins Sep 9, 2015

Member

Is there somewhere that we are explicitly needing Coveralls in this class?

lib/slather/project.rb
+ if profdata_coverage_dir == nil || (coverage_profdata = Dir["#{profdata_coverage_dir}/**/Coverage.profdata"].first) == nil
+ raise StandardError, "No Coverage.profdata files found. Please make sure the \"Code Coverage\" checkbox is enabled in your scheme's Test action or the build_directory property is set."
+ end
+ xcode_path = `xcode-select -p`.strip

This comment has been minimized.

@pietbrauer

pietbrauer Sep 11, 2015

Contributor

I had to add .gsub(/ /, '\ ') to fix the current path as the GM of Xcode 7: /Applications/Xcode 2.app/Contents/Developer/ has a space in it. This transforms it to be /Applications/Xcode\\ 2.app/Contents/Developer/

@pietbrauer

pietbrauer Sep 11, 2015

Contributor

I had to add .gsub(/ /, '\ ') to fix the current path as the GM of Xcode 7: /Applications/Xcode 2.app/Contents/Developer/ has a space in it. This transforms it to be /Applications/Xcode\\ 2.app/Contents/Developer/

This comment has been minimized.

@AliSoftware

AliSoftware Sep 17, 2015

Contributor

@pietbrauer You should use shellescape instead of adding a gsub to fix that

@AliSoftware

AliSoftware Sep 17, 2015

Contributor

@pietbrauer You should use shellescape instead of adding a gsub to fix that

@dyundt

This comment has been minimized.

Show comment
Hide comment
@dyundt

dyundt Sep 11, 2015

Has this been merged to master as of yet? If not when will it be? Im very interested in using slather with the Coverage.profdata file to generate cobertura reports in jenkins.

dyundt commented Sep 11, 2015

Has this been merged to master as of yet? If not when will it be? Im very interested in using slather with the Coverage.profdata file to generate cobertura reports in jenkins.

@szweier

This comment has been minimized.

Show comment
Hide comment
@szweier

szweier Sep 17, 2015

Interested in this as well, any info on this being merged.

szweier commented Sep 17, 2015

Interested in this as well, any info on this being merged.

lib/slather/project.rb
+ end
+ xcode_path = `xcode-select -p`.strip
+ llvm_cov_command = File.join(xcode_path, "Toolchains/XcodeDefault.xctoolchain/usr/bin/llvm-cov show -instr-profile #{coverage_profdata} #{binary_file}")
+ `#{llvm_cov_command}`

This comment has been minimized.

@stevepeak

stevepeak Sep 21, 2015

Contributor

@pietbrauer Nice job on putting this together. I've extracted some of the concepts here and added them directly into Codeocov's bash uploader, as seen below:

profdata=$(find ~/Library/Developer/Xcode/DerivedData -name 'Coverage.profdata' | head -1)
if [ -f "$profdata" ];
then
  _dir=$(dirname "$profdata")
  for _type in app framework xctest
  do
    _file=$(find "$_dir" -name "*.$_type" | head -1)
    if [ "$_file" != "" ];
    then
      _proj=${_file##*/}
      _proj=${_proj%."$_type"}
      xcrun llvm-cov show -instr-profile "$profdata" "$_file/$_proj" > "$_type.coverage.txt" || true
    fi
  done
fi

Extracted from codecov/codecov-bash#L367

So far a couple customers of Codecov are using this successfully. However, we are seeing some projects with 0% coverage. One customer said they fixed it by enabling Gather code coverage in xcodebuild from within xcode.

Do you know of a reason why coverage would be 0%?

PS I'm not a xcode developer so my apologies if I'm not accurate with my description, they are quotes from customer support.

@stevepeak

stevepeak Sep 21, 2015

Contributor

@pietbrauer Nice job on putting this together. I've extracted some of the concepts here and added them directly into Codeocov's bash uploader, as seen below:

profdata=$(find ~/Library/Developer/Xcode/DerivedData -name 'Coverage.profdata' | head -1)
if [ -f "$profdata" ];
then
  _dir=$(dirname "$profdata")
  for _type in app framework xctest
  do
    _file=$(find "$_dir" -name "*.$_type" | head -1)
    if [ "$_file" != "" ];
    then
      _proj=${_file##*/}
      _proj=${_proj%."$_type"}
      xcrun llvm-cov show -instr-profile "$profdata" "$_file/$_proj" > "$_type.coverage.txt" || true
    fi
  done
fi

Extracted from codecov/codecov-bash#L367

So far a couple customers of Codecov are using this successfully. However, we are seeing some projects with 0% coverage. One customer said they fixed it by enabling Gather code coverage in xcodebuild from within xcode.

Do you know of a reason why coverage would be 0%?

PS I'm not a xcode developer so my apologies if I'm not accurate with my description, they are quotes from customer support.

This comment has been minimized.

@bgerstle

bgerstle Sep 21, 2015

P.S. I'm the guy who mentioned enabling Gather coverage data (see below). You can see how we're doing it for the Wikipedia-iOS app, specifically this commit.

image

@bgerstle

bgerstle Sep 21, 2015

P.S. I'm the guy who mentioned enabling Gather coverage data (see below). You can see how we're doing it for the Wikipedia-iOS app, specifically this commit.

image

This comment has been minimized.

@AliSoftware

AliSoftware Sep 21, 2015

Contributor

@stevepeak, @bgerstle: .profdata files are only generated by Xcode if you check that "Gather coverage data". If you don't, you won't even have the Coverage computed, even inside the Xcode GUI.

So even if it's interesting to mention, that's not directly related to slather coverage itself. Even if you want the Coverage report inside Xcode itself under the Test Reports tab, you'll need to enable that anyway, even for people not using slather.

But indeed it could be interesting to make slather setup enforce this setting if it's not the case already (I haven't checked that yet).

@AliSoftware

AliSoftware Sep 21, 2015

Contributor

@stevepeak, @bgerstle: .profdata files are only generated by Xcode if you check that "Gather coverage data". If you don't, you won't even have the Coverage computed, even inside the Xcode GUI.

So even if it's interesting to mention, that's not directly related to slather coverage itself. Even if you want the Coverage report inside Xcode itself under the Test Reports tab, you'll need to enable that anyway, even for people not using slather.

But indeed it could be interesting to make slather setup enforce this setting if it's not the case already (I haven't checked that yet).

This comment has been minimized.

@bgerstle

bgerstle Sep 21, 2015

@AliSoftware do you have any idea how this is related to -enableCodeCoverage flag in xcodebuild? AFAIK that flag was introduced in Xcode 7, seems like they'd be related.

@bgerstle

bgerstle Sep 21, 2015

@AliSoftware do you have any idea how this is related to -enableCodeCoverage flag in xcodebuild? AFAIK that flag was introduced in Xcode 7, seems like they'd be related.

This comment has been minimized.

@AliSoftware

AliSoftware Sep 21, 2015

Contributor

@bgerstle haven't tested that flag yet but I'd assume it does the same thing as the checkbox in the xccheme and that's just a way to override that setting directly from the command line. But that's all just a guess.

@AliSoftware

AliSoftware Sep 21, 2015

Contributor

@bgerstle haven't tested that flag yet but I'd assume it does the same thing as the checkbox in the xccheme and that's just a way to override that setting directly from the command line. But that's all just a guess.

This comment has been minimized.

@thelvis4

thelvis4 Oct 16, 2015

@AliSoftware Yes, passing -enableCodeCoverage YES to xcodebuild does the same thing as enabling `Gather coverage data.

@thelvis4

thelvis4 Oct 16, 2015

@AliSoftware Yes, passing -enableCodeCoverage YES to xcodebuild does the same thing as enabling `Gather coverage data.

This comment has been minimized.

@Legoless

Legoless Oct 27, 2015

Note that if you're using CI, you need to explicitly pass enableCodeCoverage to xcodebuild (apparently the setting in the scheme gets ignored for some reason).

@Legoless

Legoless Oct 27, 2015

Note that if you're using CI, you need to explicitly pass enableCodeCoverage to xcodebuild (apparently the setting in the scheme gets ignored for some reason).

This comment has been minimized.

@jkrumow

jkrumow Oct 27, 2015

Collaborator

@Legoless Event if you set the scheme to "shared"?

@jkrumow

jkrumow Oct 27, 2015

Collaborator

@Legoless Event if you set the scheme to "shared"?

This comment has been minimized.

@Legoless

Legoless Oct 27, 2015

Shared should solve the issue. I have lots of other issues though. Such as: error: Failed to load coverage: No object file for requested architecture

@Legoless

Legoless Oct 27, 2015

Shared should solve the issue. I have lots of other issues though. Such as: error: Failed to load coverage: No object file for requested architecture

@marklarr

This comment has been minimized.

Show comment
Hide comment
@marklarr

marklarr Sep 21, 2015

Contributor

should we close this out and move discussion to #99?

Contributor

marklarr commented Sep 21, 2015

should we close this out and move discussion to #99?

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Dec 27, 2015

Contributor

So, these commits add support for the binary-file param, as per @larslockefeer proposal. Any feedback is highly appreciated :)

Contributor

viteinfinite commented Dec 27, 2015

So, these commits add support for the binary-file param, as per @larslockefeer proposal. Any feedback is highly appreciated :)

@larslockefeer

This comment has been minimized.

Show comment
Hide comment
@larslockefeer

larslockefeer Dec 28, 2015

@viteinfinite Awesome, thanks! I'll check this out later today.

@viteinfinite Awesome, thanks! I'll check this out later today.

@richardbuckle

This comment has been minimized.

Show comment
Hide comment
@richardbuckle

richardbuckle Dec 28, 2015

Just as a data point, I have today built and installed this branch of the gem locally on my Jenkins server and it is generating Cobertura reports very nicely. Thanks for all the hard work!

Just as a data point, I have today built and installed this branch of the gem locally on my Jenkins server and it is generating Cobertura reports very nicely. Thanks for all the hard work!

@larslockefeer

This comment has been minimized.

Show comment
Hide comment
@larslockefeer

larslockefeer Dec 29, 2015

@viteinfinite Later today turned out to be tomorrow, but I have checked it out now, it seems to be working great! Just two remarks/thoughts:

  • If you do not provide a binary file, slather will find one which is very convenient. If the binary file option is used, in the current implementation you are supposed to provide the entire path. Would it make sense to provide just the name of the binary file (so, for instance MyApp.app or MyFramework.framework) and let slather figure out the exact path? IMO, this would make using slather in CI environments less cumbersome.
  • I would like to generate one coverage report for two targets (the main target and a framework target). I can run slather twice and merge the results together, but would it make sense to accept an comma-separated list of binary files and merge the output into one report right away?

I'm more than happy to help implementing these solutions if they sound interesting.

@viteinfinite Later today turned out to be tomorrow, but I have checked it out now, it seems to be working great! Just two remarks/thoughts:

  • If you do not provide a binary file, slather will find one which is very convenient. If the binary file option is used, in the current implementation you are supposed to provide the entire path. Would it make sense to provide just the name of the binary file (so, for instance MyApp.app or MyFramework.framework) and let slather figure out the exact path? IMO, this would make using slather in CI environments less cumbersome.
  • I would like to generate one coverage report for two targets (the main target and a framework target). I can run slather twice and merge the results together, but would it make sense to accept an comma-separated list of binary files and merge the output into one report right away?

I'm more than happy to help implementing these solutions if they sound interesting.

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Dec 30, 2015

Contributor

@larslockefeer Thank you for the feedback!
In response to your thoughts:

  1. There's probably more work to be done then - I guess we can improve this feature.
  2. Generating one coverage report for two targets can turn to be quite useful but, IMHO, it should be separate enhancement as this has never been supported in slather if I recall well. We can probably open a new enhancement issue or a PR.
Contributor

viteinfinite commented Dec 30, 2015

@larslockefeer Thank you for the feedback!
In response to your thoughts:

  1. There's probably more work to be done then - I guess we can improve this feature.
  2. Generating one coverage report for two targets can turn to be quite useful but, IMHO, it should be separate enhancement as this has never been supported in slather if I recall well. We can probably open a new enhancement issue or a PR.
@richardbuckle

This comment has been minimized.

Show comment
Hide comment
@richardbuckle

richardbuckle Jan 1, 2016

Happy New Year everyone.

Speaking as a mere user of slather I think that the further enhancements now being discussed are very worthwhile, but I wouldn't want them to cause any delay to the merging of this PR, which is IMHO good to go and removes a significant pain point for many iOS developers by allowing them to convert Xcode 7's code coverage data into the more widely understood formats. Therefore I support the idea of opening separate issues or PRs for the further enhancements so that this PR can be merged as soon as possible.

Happy New Year everyone.

Speaking as a mere user of slather I think that the further enhancements now being discussed are very worthwhile, but I wouldn't want them to cause any delay to the merging of this PR, which is IMHO good to go and removes a significant pain point for many iOS developers by allowing them to convert Xcode 7's code coverage data into the more widely understood formats. Therefore I support the idea of opening separate issues or PRs for the further enhancements so that this PR can be merged as soon as possible.

@larslockefeer

This comment has been minimized.

Show comment
Hide comment
@larslockefeer

larslockefeer Jan 4, 2016

I agree! @tarbrain requested a summary of things that need to be completed before this PR can be merged. With all the new commits, is there anything left, except for maybe updating the documentation? FWIW, the current state of this branch is working well for me.

I agree! @tarbrain requested a summary of things that need to be completed before this PR can be merged. With all the new commits, is there anything left, except for maybe updating the documentation? FWIW, the current state of this branch is working well for me.

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 4, 2016

Collaborator

Hey everybody. I will look into the PR later today. So far it looks nice.

cheers

Collaborator

jkrumow commented Jan 4, 2016

Hey everybody. I will look into the PR later today. So far it looks nice.

cheers

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jan 4, 2016

Contributor

👍

BTW, a modification of the readme is needed, as a bunch of new parameters has been added in this PR.

Contributor

viteinfinite commented Jan 4, 2016

👍

BTW, a modification of the readme is needed, as a bunch of new parameters has been added in this PR.

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 4, 2016

Collaborator

@viteinfinite I have taken a look at the specs and fixtures for simple-output, gutter, hardcover and cobertura. I saw that they still use the old gcov format and are just provisorically fixed (to handle the different byte-layout of integers is suppose).

Since we want to switch to profdata soon and deprecate the old gcov format, would you mind to update the cobertura and gutter.json specs to use the new profdata fixtures?

Cheers
tarbrain

Collaborator

jkrumow commented Jan 4, 2016

@viteinfinite I have taken a look at the specs and fixtures for simple-output, gutter, hardcover and cobertura. I saw that they still use the old gcov format and are just provisorically fixed (to handle the different byte-layout of integers is suppose).

Since we want to switch to profdata soon and deprecate the old gcov format, would you mind to update the cobertura and gutter.json specs to use the new profdata fixtures?

Cheers
tarbrain

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jan 7, 2016

Contributor

@tarbrain thanks for your feedback. I totally agree with you.
I'll start working on this next week.

Contributor

viteinfinite commented Jan 7, 2016

@tarbrain thanks for your feedback. I totally agree with you.
I'll start working on this next week.

@viteinfinite viteinfinite referenced this pull request in viteinfinite/slather Jan 18, 2016

Open

Profdata Framework Fix #11

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jan 18, 2016

Contributor

@tarbrain So here it is. These last commits modify the specs in order to test against the profdata output.

Also, a verbose mode (--verbose or -v) has been added which avoids printing the debug lines by default.

Finally, platform headers, such as Library/Frameworks/XCTest.framework/Headers/XCTestAssertionsImpl.h, are now filtered.

Contributor

viteinfinite commented Jan 18, 2016

@tarbrain So here it is. These last commits modify the specs in order to test against the profdata output.

Also, a verbose mode (--verbose or -v) has been added which avoids printing the debug lines by default.

Finally, platform headers, such as Library/Frameworks/XCTest.framework/Headers/XCTestAssertionsImpl.h, are now filtered.

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 18, 2016

Collaborator

@viteinfinite Looks sweet. I would say we all give it a try and if nobody finds a bug I will merge it soon.

Good work 🎉

Collaborator

jkrumow commented Jan 18, 2016

@viteinfinite Looks sweet. I would say we all give it a try and if nobody finds a bug I will merge it soon.

Good work 🎉

jkrumow pushed a commit that referenced this pull request Jan 19, 2016

Julian Krumow
Merge pull request #92 from viteinfinite/feature-profdata
Add support to profdata file format

@jkrumow jkrumow merged commit 7a0921b into SlatherOrg:master Jan 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 19, 2016

Collaborator

@viteinfinite Its done!
@marklarr @ayanonagon You can release a new gem version 👍

Collaborator

jkrumow commented Jan 19, 2016

@viteinfinite Its done!
@marklarr @ayanonagon You can release a new gem version 👍

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jan 20, 2016

Contributor

🎉🎉🎉

Contributor

viteinfinite commented Jan 20, 2016

🎉🎉🎉

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Jan 26, 2016

Contributor

@tarbrain BTW, gem's not pushed yet?

Contributor

viteinfinite commented Jan 26, 2016

@tarbrain BTW, gem's not pushed yet?

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 26, 2016

Collaborator

I can not push the gem since I do not belong to the developers at venmo.

We should ask @marklarr or @ayanonagon again.

Collaborator

jkrumow commented Jan 26, 2016

I can not push the gem since I do not belong to the developers at venmo.

We should ask @marklarr or @ayanonagon again.

@ayanonagon

This comment has been minimized.

Show comment
Hide comment
@ayanonagon

ayanonagon Jan 26, 2016

Contributor

@tarbrain Is your email the one listed on GitHub? If so I’ll add you as an owner to the gem. Thanks for helping us out. We really appreciate it! 😃

Contributor

ayanonagon commented Jan 26, 2016

@tarbrain Is your email the one listed on GitHub? If so I’ll add you as an owner to the gem. Thanks for helping us out. We really appreciate it! 😃

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Jan 26, 2016

Collaborator

@ayanonagon @marklarr Thanks for adding me to the list.

But please do not rely on me for maintaining this repository. I wont have the time. I am just one of the many professional slather users who are waiting for the important profdata feature and feel somewhat let down by the venmo developers. You have a big user base and you should take responsibility for that.

Regards
tarbrain

Collaborator

jkrumow commented Jan 26, 2016

@ayanonagon @marklarr Thanks for adding me to the list.

But please do not rely on me for maintaining this repository. I wont have the time. I am just one of the many professional slather users who are waiting for the important profdata feature and feel somewhat let down by the venmo developers. You have a big user base and you should take responsibility for that.

Regards
tarbrain

@richardbuckle

This comment has been minimized.

Show comment
Hide comment
@richardbuckle

richardbuckle Jan 27, 2016

As a user, I do hope that the gem will be pushed soon. It has been quite a long wait.

I've been holding off on publishing a blog post about getting Xcode 7 to play nice with Jenkins because I want my instructions for installing slather to be simply gem install slather. It doesn't feel right to send people to a fork of the repo but maybe that would be better than waiting much longer.

As a user, I do hope that the gem will be pushed soon. It has been quite a long wait.

I've been holding off on publishing a blog post about getting Xcode 7 to play nice with Jenkins because I want my instructions for installing slather to be simply gem install slather. It doesn't feel right to send people to a fork of the repo but maybe that would be better than waiting much longer.

@ayanonagon

This comment has been minimized.

Show comment
Hide comment
@ayanonagon

ayanonagon Feb 3, 2016

Contributor

@tarbrain We’re very sorry for the wait. It’s clear that we haven’t done a great job in maintaining this project. The main issues are that nobody on the current iOS team is fluent in Ruby and also that we are not currently using this project in our own workflow. It’s unfair to make other developers wait because of us, so we apologize again for that.

On a happier note, our friend @neonichu has graciously offered to pick up maintaining the project. As you might have noticed, we pulled it out into a separate GitHub organization as well. Anyway, slather is in more capable hands now, so we’re excited to see where the community takes it. 🎉

Thanks!

Contributor

ayanonagon commented Feb 3, 2016

@tarbrain We’re very sorry for the wait. It’s clear that we haven’t done a great job in maintaining this project. The main issues are that nobody on the current iOS team is fluent in Ruby and also that we are not currently using this project in our own workflow. It’s unfair to make other developers wait because of us, so we apologize again for that.

On a happier note, our friend @neonichu has graciously offered to pick up maintaining the project. As you might have noticed, we pulled it out into a separate GitHub organization as well. Anyway, slather is in more capable hands now, so we’re excited to see where the community takes it. 🎉

Thanks!

@AliSoftware

This comment has been minimized.

Show comment
Hide comment
@AliSoftware

AliSoftware Feb 3, 2016

Contributor

@ayanonagon never apologize for doing OpenSource and giving such a great tool to the community for free 😉

@neonichu thx for picking up on the project maintenance 🎉

Contributor

AliSoftware commented Feb 3, 2016

@ayanonagon never apologize for doing OpenSource and giving such a great tool to the community for free 😉

@neonichu thx for picking up on the project maintenance 🎉

@neonichu

This comment has been minimized.

Show comment
Hide comment
Member

neonichu commented Feb 3, 2016

@neonichu neonichu referenced this pull request Feb 3, 2016

Closed

Swift test coverage. #39

@neonichu

This comment has been minimized.

Show comment
Hide comment
@neonichu

neonichu Feb 3, 2016

Member

Allright, created a milestone for 2.0 https://github.com/SlatherOrg/slather/milestones/2.0.0, once the things in there are addressed, we can :shipit:

Member

neonichu commented Feb 3, 2016

Allright, created a milestone for 2.0 https://github.com/SlatherOrg/slather/milestones/2.0.0, once the things in there are addressed, we can :shipit:

@viteinfinite

This comment has been minimized.

Show comment
Hide comment
@viteinfinite

viteinfinite Feb 4, 2016

Contributor

@ayanonagon thanks for the update.
@neonichu, may your crafty hands save us from bugs dues to uncovered code.

Contributor

viteinfinite commented Feb 4, 2016

@ayanonagon thanks for the update.
@neonichu, may your crafty hands save us from bugs dues to uncovered code.

@jkrumow

This comment has been minimized.

Show comment
Hide comment
@jkrumow

jkrumow Feb 4, 2016

Collaborator

@ayanonagon @neonichu
Thank you for the response! I think making slather an organization is a good decision and makes it easier to participate. Very good!

Collaborator

jkrumow commented Feb 4, 2016

@ayanonagon @neonichu
Thank you for the response! I think making slather an organization is a good decision and makes it easier to participate. Very good!

@yas375 yas375 referenced this pull request May 1, 2016

Closed

��� in results #199

@drewcrawford drewcrawford referenced this pull request in AnarchyTools/atbuild Jul 14, 2016

Open

Code coverage #30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment