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.

Sign up for GitHub

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

Jump to bottom

[#496] multiple outputs #893

Merged
merged 12 commits into from
Nov 11, 2022
Merged

[#496] multiple outputs #893

merged 12 commits into from
Nov 11, 2022

Conversation

cemoktra
Copy link
Contributor

@cemoktra cemoktra commented Sep 28, 2022 •
edited
Loading

Solve #496

@ahollmann ahollmann mentioned this pull request Oct 4, 2022
Closed
@marco-c marco-c linked an issue Oct 10, 2022 that may be closed by this pull request
Improvement: multiple output types #496
Closed
marco-c
marco-c reviewed Oct 10, 2022
View reviewed changes
src/cobertura.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Oct 11, 2022
View reviewed changes
src/main.rs Show resolved Hide resolved
marco-c
marco-c reviewed Oct 11, 2022
View reviewed changes
src/cobertura.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Oct 31, 2022
View reviewed changes
src/cobertura.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Oct 31, 2022
View reviewed changes
src/cobertura.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Oct 31, 2022
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Oct 31, 2022
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 4, 2022
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 4, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 4, 2022
View reviewed changes
src/output.rs Show resolved Hide resolved
marco-c
marco-c reviewed Nov 4, 2022
View reviewed changes
src/cobertura.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/html.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/llvm_tools.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/main.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/output.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/parser.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/parser.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/parser.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/path_rewriting.rs Outdated Show resolved Hide resolved
marco-c
marco-c reviewed Nov 7, 2022
View reviewed changes
src/path_rewriting.rs Outdated Show resolved Hide resolved
@marco-c
Copy link
Collaborator

marco-c commented Nov 11, 2022

LGTM, could you run the output benchmarks before/after your change?

@cemoktra
Copy link
Contributor Author

LGTM, could you run the output benchmarks before/after your change?

how can i do this?

@marco-c
Copy link
Collaborator

marco-c commented Nov 11, 2022

You can use cargo bench to run the benchmarks.

@cemoktra
Copy link
Contributor Author

Okay, Well they don't even compile at the moment. Will fix this

@cemoktra
Copy link
Contributor Author

Also they are supposed to only work on nightly?

@cemoktra
Copy link
Contributor Author

Main branch:

test bench_filter_covered                          ... bench:           6 ns/iter (+/- 0)
test bench_filter_covered_functions_executed       ... bench:           5 ns/iter (+/- 0)
test bench_filter_covered_no_functions             ... bench:           5 ns/iter (+/- 1)
test bench_filter_covered_toplevel_executed        ... bench:           6 ns/iter (+/- 0)
test bench_filter_uncovered_functions_not_executed ... bench:           8 ns/iter (+/- 1)
test bench_filter_uncovered_no_lines_executed      ... bench:          10 ns/iter (+/- 1)
test bench_lib_consumer      ... bench:   8,346,323 ns/iter (+/- 820,814)
test bench_lib_merge_results ... bench:         285 ns/iter (+/- 77)
test bench_output_activedata_etl ... bench:       7,194 ns/iter (+/- 1,425)
test bench_output_covdir         ... bench:      46,385 ns/iter (+/- 6,972)
test bench_output_lcov           ... bench:      46,331 ns/iter (+/- 7,053)
test bench_parser_gcov   ... bench:   1,373,165 ns/iter (+/- 106,741)
test bench_parser_jacoco ... bench:  18,538,181 ns/iter (+/- 1,167,574)
test bench_parser_lcov   ... bench:  19,656,973 ns/iter (+/- 1,524,191)
test bench_reader_finalize_file        ... bench:       2,028 ns/iter (+/- 368)
test bench_reader_finalize_file_branch ... bench:       2,024 ns/iter (+/- 375)
test bench_reader_gcda                 ... bench:         122 ns/iter (+/- 17)
test bench_reader_gcno                 ... bench:       3,916 ns/iter (+/- 1,116)
test bench_reader_gcno_dump            ... bench:      10,132 ns/iter (+/- 1,361)
test bench_reader_gcno_gcda_dump       ... bench:      11,343 ns/iter (+/- 1,070)

This branch:

test bench_filter_covered                          ... bench:           6 ns/iter (+/- 0)
test bench_filter_covered_functions_executed       ... bench:           6 ns/iter (+/- 1)
test bench_filter_covered_no_functions             ... bench:           4 ns/iter (+/- 0)
test bench_filter_covered_toplevel_executed        ... bench:           7 ns/iter (+/- 0)
test bench_filter_uncovered_functions_not_executed ... bench:           8 ns/iter (+/- 0)
test bench_filter_uncovered_no_lines_executed      ... bench:          10 ns/iter (+/- 0)
test bench_lib_consumer      ... bench:   8,228,188 ns/iter (+/- 553,026)
test bench_lib_merge_results ... bench:         281 ns/iter (+/- 26)
test bench_output_activedata_etl ... bench:       6,789 ns/iter (+/- 973)
test bench_output_covdir         ... bench:      45,502 ns/iter (+/- 4,446)
test bench_output_lcov           ... bench:      44,477 ns/iter (+/- 4,604)
test bench_parser_gcov   ... bench:   1,378,784 ns/iter (+/- 125,122)
test bench_parser_jacoco ... bench:  18,535,355 ns/iter (+/- 1,384,340)
test bench_parser_lcov   ... bench:  20,023,440 ns/iter (+/- 1,685,581)
test bench_reader_finalize_file        ... bench:       1,980 ns/iter (+/- 114)
test bench_reader_finalize_file_branch ... bench:       1,945 ns/iter (+/- 393)
test bench_reader_gcda                 ... bench:         122 ns/iter (+/- 14)
test bench_reader_gcno                 ... bench:       3,895 ns/iter (+/- 359)
test bench_reader_gcno_dump            ... bench:      10,472 ns/iter (+/- 2,196)
test bench_reader_gcno_gcda_dump       ... bench:      11,960 ns/iter (+/- 2,223)

so no real difference

@marco-c
Copy link
Collaborator

marco-c commented Nov 11, 2022

Perfect, thanks!

marco-c
marco-c approved these changes Nov 11, 2022
View reviewed changes
Copy link
Collaborator

@marco-c marco-c left a comment

Choose a reason for hiding this comment

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

Thanks!

@marco-c marco-c merged commit 4b821e9 into mozilla:master Nov 11, 2022
@sylvestre
Copy link
Contributor

It would have been nice to keep the output-type args.
This change seems to have broke existing usage:

error: Found argument '--output-type' which wasn't expected, or isn't valid in this context
Error: 	Did you mean --output-types?

USAGE:
    grcov.exe <paths>... --output-types <OUTPUT TYPE>... <--token <TOKEN>|--service-job-id <SERVICE JOB ID>>

For more information try --help
Error: Process completed with exit code 1.

sylvestre added a commit to sylvestre/coreutils that referenced this pull request Nov 12, 2022
@sylvestre
coverage: rename the arg to match a change in grcov
6996225 
See mozilla/grcov#893
@sylvestre sylvestre mentioned this pull request Nov 12, 2022
Merged
@marco-c
Copy link
Collaborator

marco-c commented Nov 12, 2022

Good point, we should add support for the old argument as well (maybe marking it as deprecated?) before releasing the new version.

@marco-c
Copy link
Collaborator

marco-c commented Nov 12, 2022

I filed #922. @cemoktra are you interested in working on it?

@cemoktra
Copy link
Contributor Author

Depends on how urgent it is. Not sure if I find time over the next week

@cemoktra cemoktra deleted the multiple-outputs branch November 13, 2022 20:08
@cemoktra cemoktra mentioned this pull request Nov 13, 2022
Merged
@cemoktra
Copy link
Contributor Author

i found time and created #923 as it is hopefully a fairly simple thing

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

@marco-c marco-c marco-c approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improvement: multiple output types
3 participants
@cemoktra @marco-c @sylvestre