julia-actions / julia-runtest

This action runs the tests in a Julia package.
MIT License
57 stars 26 forks source link

Pass custom arguments? #47

Open johnomotani opened 3 years ago

johnomotani commented 3 years ago

It would be nice to be able to pass some arguments to control tests. My use case is that I have some extra tests (that take a long-ish time), that I would like to run only on PR, not on push. If I had the option either to pass --long to julia on the command line or to give a test_args=["--long"] argument to Pkg.test() then this would be possible. I'm not sure if either is better or more convenient to implement. I'm not a Github Actions expert, but maybe a test_args option under input could be used to allow passing extra arguments like this?

SaschaMann commented 3 years ago

@DilumAluthge you implemented this in #39 but then closed the PR due to its complexity. Do you remember why it was that complicated? Looking at it, it seems like your PR already contains the logic to make this work but I'm probably missing something.


Unrelated to this PR in particular, I'm a bit worried about all the complexity added to this action over time. I personally feel like it'd be better to just call Pkg.test manually in the workflow instead of using this action if the workflow requires a lot of customisation.

(this doesn't mean this request won't be implemented)

johnomotani commented 3 years ago

At a quick glance, this could be simpler than #39, because the request is only for the test_args argument to Pkg.test(), not arbitrary arguments. Although I take the point that it might be better to just call Pkg.test() explicitly in the action for more complex cases.

dgleich commented 2 years ago

Just a quick note with an alternative in case anyone else ends up here via google. Since this still isn't merged / the functionality isn't there (and I appreciate the complexity of adding what seem like simple things!), I ended up specifying these args via an environment variable JULIA_ACTIONS_RUNTEST_ARGS and then adding

# TODO, improve quotes, etc. here... 
# Use environment variables to pass arguments from julia-actions/runtests 
# we just push these along... keep it simple. 
envargs = get(()->"", ENV, "JULIA_ACTIONS_RUNTEST_ARGS")
foreach(s->push!(ARGS,s), split(envargs,","))

to my runtests.jl file. It's a workable alternative that may suffice for other people (and was much easier for me than writing the Julia call myself...!)

kellertuer commented 2 years ago

Just to add my 2cents to the complexity discussion (then).

Now that I am trying to magically cook a CI that does (some) tests with compiler options, I am much in favour of this action, since it makes things sooooo simple. Still, if it would be possible to mirror a few more of the Julia command line options that would be really great.

DilumAluthge commented 2 years ago

I'm also concerned about the complexity we're adding to this Action, which is why I closed #39. I think that in these use cases, you should just have your own run step that runs Julia and calls Pkg.test. Then you have maximum control.

The only downside of calling Pkg.test yourself (instead of using this action) is that you don't get the automatic application of force_latest_compatible_version on CompatHelper PRs. (See https://github.com/JuliaRegistries/CompatHelper.jl/issues/298 for context.)

The solution to that is not to add more complexity to this action. Instead, the best approach will be for someone to create a small standalone GitHub Action that simply:

  1. Computes whether or not this is a CompatHelper PR.
  2. If it is, set an output from the action.

Then, in your workflow file, you would take the output from that action and pass it as the value of the force_latest_compatible_version keyword argument in your custom invocation of Pkg.test.

Once such an Action exists, there will be no advantage to using julia-actions/julia-runtest over your custom Pkg.test invocation, and at that point, I would say we will be able to close this issue as "not planned".

SaschaMann commented 2 years ago

@DilumAluthge While I generally agree with you, I think your comment also illustrates that as a general user, who might mostly rely on workflow templates and the likes, this feature might be benefitial. Setting and using outputs is something that probably goes beyond what most people encounter if all they do with GHA is running tests and maybe deploying docs. Not that it's complicated but it requires more GHA-specific knowledge and understanding.

kellertuer commented 2 years ago

concerning the complicated – I (as an not-really-experienced bash/GHA person) find that quite complicated. It took me a day to get a custom test run to work, but it is still not reporting the code coverage (for the custom runs, only for the matrix group case that still runs the classical test from you) and I have no clue why, so I might just again give up (on my try to speed up tests). If you want to take a look its this one

https://github.com/JuliaManifolds/Manifolds.jl/pull/507/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f

and in the PR you see, I mainly do copy paste & guesswork (because, sure I miss the understanding).

SaschaMann commented 2 years ago

concerning the complicated – I (as an not-really-experienced bash/GHA person) find that quite complicated. It took me a day to get a custom test run to work, but it is still not reporting the code coverage

That's what I was trying to say. For people who are experienced with GHA already, Dilum's suggestion works fine. But most users of the Julia CI actions aren't familiar enough with it already so they have to figure out how GHA works, learn some concepts of it etc. which adds a lot of friction. Or try to copy and paste it together, which is also rather frustrating.

I'll see if there's a decent way to add a custom arg input of some sort to the action that doesn't add too much complexity to the action itself.

kellertuer commented 2 years ago

I'll see if there's a decent way to add a custom arg input of some sort to the action that doesn't add too much complexity to the action itself.

Perfect. I personally would be fine with a possibility to set -O and --compile, but I understand that this is completely subjective and if everyone asks, there is a lot of command line options to individually pass.

DilumAluthge commented 2 years ago

I don't think it's sustainable to add an individual input to this action for every different configuration option. If we do add this feature, I think the only sustainable path is to have a single "custom args" input that can take in arbitrary command-line flags.

kellertuer commented 2 years ago

Oh, in what is complicated and what not, I trust your evaluation (sure every option individually is a lot of code). Thanks for thinking about this extension – and for prodiving the action in the first place, it helps a lot!

edit: So for my own approach I am now out of ideas. I can not get a custom run (since I would like to use compiler options) to generate the code coverage (the action to collect that reports it does not exist on 1.7 where we do the collection). So while this in theory would speed up our tests by a factor 2 or 3, in practice I am not clever enough to get this to work and would be very happy for the option here, because otherwise, we will just not have that (the clumsy PR which you can see is mainly guesswork after the first version where I still though I would understand anything is https://github.com/JuliaManifolds/Manifolds.jl/pull/507)