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

feat(coverage): add "--output" flag #13289

Merged

Conversation

VishnuJin
Copy link
Contributor

@VishnuJin VishnuJin commented Jan 5, 2022

This pr adds output arg to deno coverage to pipe the stdout to the given file, just like deno bundle

fixes #13236

@VishnuJin
Copy link
Contributor Author

VishnuJin commented Jan 5, 2022

@bartlomieju
I believe deno bundle accepts only one source file and hence the second arg can be interpreted as out_file but in case of deno coverage it accepts multiple files, how can we assume that the last arg is the out_file argument unless explicitly specified with out_file ?

@VishnuJin VishnuJin marked this pull request as draft January 5, 2022 14:52
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch from 76f1b5e to eb69f8a Compare January 5, 2022 14:56
@bartlomieju
Copy link
Member

bartlomieju commented Jan 6, 2022

@bartlomieju I believe deno bundle accepts only one source file and hence the second arg can be interpreted as out_file but in case of deno coverage it accepts multiple files, how can we assume that the last arg is the out_file argument unless explicitly specified with out_file ?

Good point. Let's go with explicit --output argument.

@VishnuJin
Copy link
Contributor Author

@bartlomieju apologies for the delay!! I have resumed working on this and can share progress soon..

@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch from 5143049 to fbc5b4c Compare January 23, 2022 08:06
@VishnuJin VishnuJin marked this pull request as ready for review January 23, 2022 08:06
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch 3 times, most recently from 5d916f1 to 72ce3d2 Compare January 23, 2022 08:37
cli/tools/coverage/mod.rs Outdated Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/tools/coverage/mod.rs Outdated Show resolved Hide resolved
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch 2 times, most recently from 290271f to 1828e2d Compare January 27, 2022 17:40
@bartlomieju bartlomieju changed the title feat(cli) add 'out_file' arg to deno coverage feat(coverage): add "--output" flag Jan 31, 2022
cli/tools/coverage/mod.rs Outdated Show resolved Hide resolved
cli/tools/coverage/mod.rs Outdated Show resolved Hide resolved
cli/tools/coverage/mod.rs Outdated Show resolved Hide resolved
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch from 7e8b0b8 to f242a32 Compare January 31, 2022 17:13
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch 3 times, most recently from c61385e to 51bf12c Compare January 31, 2022 19:20
@VishnuJin
Copy link
Contributor Author

@bartlomieju I noticed js_unit_tests tests infastci macOS fail couple of time although it passed in my local runs

@bartlomieju
Copy link
Member

@VishnuJin no worries, seems like a flake.

@VishnuJin
Copy link
Contributor Author

@bartlomieju awaiting for you review 🙂

cli/flags.rs Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
cli/flags.rs Outdated Show resolved Hide resolved
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch 3 times, most recently from 3203842 to eff2796 Compare February 11, 2022 17:28
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@VishnuJin I just tested this PR and it works nicely, we should land it for 1.19. I found one quirk though, please see comment below

@@ -732,6 +733,17 @@ Generate html reports from lcov:
.help("Output coverage report in lcov format")
.takes_value(false),
)
.arg(Arg::new("output")
.long("output")
Copy link
Member

Choose a reason for hiding this comment

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

This arg should require --lcov arg to be present, otherwise I can do:

deno coverage --output=cov.lcov cov/

which makes no sense, since it means to print test report to stdout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I somehow missed requires method in clap and implemented the following which is obviously not so good, and I will change this now

// only create file for if '--lcov' flag is passed
let mut out_mode: Option<PathBuf> = None;
if coverage_flags.lcov {
out_mode = match coverage_flags.output {
Some(ref path) => match File::create(path) {
Ok(_) => Some(PathBuf::from(path)),
Err(e) => {
return Err(anyhow!("Failed to create output file: {}", e));
}
},
None => None,
};
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bartlomieju thanks for the hint,

deno/cli/flags.rs

Lines 736 to 738 in c4356c0

.arg(Arg::new("output")
.requires("lcov")
.long("output")

Signed-off-by: VishnuJin <vishnujin@outlook.com>
@VishnuJin VishnuJin force-pushed the feat(cli)-add-output-arg-to-coverage branch from eff2796 to c4356c0 Compare February 15, 2022 05:12
@lucacasonato lucacasonato added this to the 1.19.0 milestone Feb 15, 2022
@VishnuJin
Copy link
Contributor Author

@bartlomieju should I do something here ?

@bartlomieju
Copy link
Member

@VishnuJin no, I made a mistake rebasing your PR, need to land other PR first before it's fixed.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @VishnuJin

@bartlomieju bartlomieju merged commit 2dc5dba into denoland:main Feb 15, 2022
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.

Add output argument to deno coverage when used with --lcov
3 participants