-
Notifications
You must be signed in to change notification settings - Fork 16
Download and run cargo-tarpaulin from GitHub releases #1
Conversation
068549d
to
4daad73
Compare
df7c5c1
to
2441bf6
Compare
33e1876
to
f75a8fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I dropped a few thoughts and ideas, they are not mandatory, but either worth to add or at least could be discussed.
README.md
Outdated
|
||
As `rustup` is not installed by default for [macOS environments](https://help.github.com/en/articles/virtual-environments-for-github-actions) | ||
at the moment (2019-09-13), this Action will try its best to install it before any other operations. | ||
* `version`: The version of `cargo-tarpaulin` that will be installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to make the inputs table, same as you suggested for actions-rs/cargo
and other Actions.
README.md
Outdated
* `version`: The version of `cargo-tarpaulin` that will be installed. | ||
* `run-types`: The type of tests to run (`Tests`, or `Doctests`). Runs all by default. May be overridden by `args`. | ||
* `timeout`: The timeout, in seconds, before cancelling execution of a long running test. May be overriden by `args`. | ||
* `tarpaulin-args`: Extra command line arguments that are passed to `cargo-tarpaulin`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be renamed to args
in order to be the same as actions-rs/cargo
args
input? By making it consistent we could provide a better experience for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, afraid not. I'd already tried this. It seems like args
is hijacked by the actions runtime (for the purpose of passing it to containers).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, it works for actions-rs/cargo
somehow.
Maybe it should be declared in action.yml
too in order to be used?
README.md
Outdated
at the moment (2019-09-13), this Action will try its best to install it before any other operations. | ||
* `version`: The version of `cargo-tarpaulin` that will be installed. | ||
* `run-types`: The type of tests to run (`Tests`, or `Doctests`). Runs all by default. May be overridden by `args`. | ||
* `timeout`: The timeout, in seconds, before cancelling execution of a long running test. May be overriden by `args`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, run-types
and timeout
are most used arguments for tarpaulin
executable? Not sure if it worth to put them as a separate inputs. actions-rs/grcov
is using a separate config file, as the grcov
has a lot of flags and options to tune, but I now think that it was a bad idea and it should probably just use the args
input.
I would love to hear your thoughts about it, as it is not clear for me, how should such Actions behave at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure about this, but I'm going to have a think about it. I think there's some cases where a "user friendly" option could be better, but then maybe the best place for that is in the upstream package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming back to this, I think allowing an args
input and special casing some of the more common inputs makes sense (to make them more visible where a program has a lot of options). Otherwise, we'd need to keep adding new inputs to the config file as the programs grow more features and options.
- name: Upload to codecov.io | ||
uses: codecov/codecov-action@v1.0.2 | ||
with: | ||
token: ${{secrets.CODECOV_TOKEN}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kinda unclear how does tarpaulin
-generated file is read by codecov
Action?
It seems that tarpaulin
generates file at ./cobertura.xml
and codecov-action
is using the same path by default, but it is unobvious, could we add them explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The codecov
action just scans all files in the cwd (using find
and grep
) to identify coverage files, so you don't have to match up paths. Tarpaulin currently doesn't have an option for the output path AFAIK.
action.yml
Outdated
description: 'The type of the coverage run [possible values: Tests, Doctests]' | ||
required: false | ||
|
||
github-release-endpoint: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the cases when this input could be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local testing only when I want to mock the GitHub API, maybe it should be something other than an input.
src/main.ts
Outdated
args.push('--timeout', config.timeout); | ||
} | ||
|
||
additionalArgs.forEach(item => args.push(item)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not like I am a JS/TS expert, but should not the args = args.concat(additionalArgs)
do the same thing?
.idea/httpRequests | ||
|
||
# Android studio 3.1+ serialized cache file | ||
.idea/caches/build_file_checksums.ser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the IDE-related stuff should go to your global gitignore probably, as it seems unreasonable to maintain the same ignore lists for all IDE used for each repo
src/config.ts
Outdated
let additionalOptions: string[] = []; | ||
|
||
if (input.opts !== null) { | ||
let opts = input.opts.match(/[^\s]+|"(?:\\"|[^"])+"/g); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used string-argv
npm package to parse CLI arguments string in actions-rs/cargo
, we could use it here too, as it should handle any edge cases better
Any update on this pull request? It would be very helpful to have this 👍 (if I get time, I might be able to help in a couple of weeks if this is still pending). |
Hey @svartalf, could you re-review? Would be great to have this! |
This is mostly a draft right now, but should be fully functioning (though you'll have to invoke
cargo tarpaulin
yourself, this action is only installing it right now). Some things still to do:dist/index.js
, only sources are in here now (maybe we add a pre-commit hook for this)cargo-tarpaulin
versions (if someone requests an invalid version, we should be able to identify that)latest
version to the current GitHub release.cargo-tarpaulin
as part of the action.We're using the version that is published to tarpaulins GitHub releases page instead of pulling it from crates.io. This cuts a considerable amount of runtime off of the job (10m vs 1-5s). This works because the GitHub actions virtual environment is identical (for all intents and purposes, they're both using Ubuntu latest) to the environment that tarpaulin is being built in.