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

Pass custom arguments? #47

Closed
johnomotani opened this issue Jul 8, 2021 · 11 comments · Fixed by #118
Closed

Pass custom arguments? #47

johnomotani opened this issue Jul 8, 2021 · 11 comments · Fixed by #118

Comments

@johnomotani
Copy link

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
Copy link
Member

@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
Copy link
Author

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
Copy link

dgleich commented Apr 21, 2022

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
Copy link

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
Copy link
Member

DilumAluthge commented Jul 22, 2022

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 JuliaRegistries/CompatHelper.jl#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
Copy link
Member

SaschaMann commented Jul 23, 2022

@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
Copy link

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
Copy link
Member

SaschaMann commented Jul 23, 2022

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
Copy link

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
Copy link
Member

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
Copy link

kellertuer commented Jul 24, 2022

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 JuliaManifolds/Manifolds.jl#507)

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 a pull request may close this issue.

5 participants