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

Add julia test CI #120

Merged
merged 7 commits into from
Apr 5, 2021
Merged

Add julia test CI #120

merged 7 commits into from
Apr 5, 2021

Conversation

danielolsen
Copy link
Contributor

@danielolsen danielolsen commented Apr 5, 2021 •
edited
Loading

Purpose

Addresses #80.

What is the code doing

On push or PR, runs tests on Julia versions {1.5, latest}. File generated using PkgTemplates.jl, default settings for PkgTemplates.GitHubActions, with Julia version 1.0 removed manually from the version matrix (our current dependencies are apparently not compatible with 1.0, see https://github.com/Breakthrough-Energy/REISE.jl/runs/2267145710).

Time to review

5 minutes? I don't understand github CI yaml very well but you probably do. Maybe we want to only test on push to match the python tests?

@danielolsen danielolsen requested a review from jenhagg April 5, 2021 03:30
@danielolsen danielolsen self-assigned this Apr 5, 2021
@danielolsen danielolsen force-pushed the daniel/julia_test_ci branch from 7573a36 to 7924072 Compare April 5, 2021 03:31
@jenhagg
Copy link
Collaborator

jenhagg commented Apr 5, 2021

The trade off with pull_request trigger is we get duplicate runs within a PR, but it's required if we want actions to run on PR's from forks. I might leave it out for now unless we expect external contributions, but either way is fine.

jenhagg
jenhagg reviewed Apr 5, 2021
View reviewed changes
.github/workflows/test-julia.yml Outdated
@@ -0,0 +1,36 @@
name: CI
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should either have the names reflect julia/python, or combine the jobs into one workflow called "CI". I'm leaning slightly toward the latter, any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I guess I forgot to push that change. I had changed it to Test locally, but never committed/pushed.

Combined testing jobs are fine with me. So we would just have test.yml, with name ____, with two jobs names e.g. test-python and test-julia?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, sounds good! I suppose naming it Test also makes more sense than CI.

@danielolsen
Copy link
Contributor Author

The trade off with pull_request trigger is we get duplicate runs within a PR, but it's required if we want actions to run on PR's from forks. I might leave it out for now unless we expect external contributions, but either way is fine.

I'm fine with leaving it on push only until we get our first external PR, if/when that happens we can update the settings and ask them to rebase to the latest.

jenhagg
jenhagg reviewed Apr 5, 2021
View reviewed changes
.github/workflows/test-julia.yml Outdated
arch: ${{ matrix.arch }}
- uses: actions/cache@v1
env:
cache-name: cache-artifacts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need both cache-name and the -test- component? I'm thinking these could be combined/shortened. Maybe something like ${{ runner.os }}-artifacts-${{ hashFiles('**/Project.toml') }}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea. I'll change it per your suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

jenhagg
jenhagg reviewed Apr 5, 2021
View reviewed changes
.github/workflows/test-julia.yml Outdated
key: ${{ runner.os }}-test-${{ env.cache-name }}-${{ hashFiles('**/Project.toml') }}
restore-keys: |
${{ runner.os }}-test-${{ env.cache-name }}-
${{ runner.os }}-test-
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the last two keys are probably unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I trust you, I have no idea what these do. I will remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

jenhagg
jenhagg approved these changes Apr 5, 2021
View reviewed changes
Copy link
Collaborator

@jenhagg jenhagg left a comment

Choose a reason for hiding this comment

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

This is great, thanks for setting it up.

@danielolsen danielolsen merged commit d274cca into develop Apr 5, 2021
@danielolsen danielolsen deleted the daniel/julia_test_ci branch April 5, 2021 22:58
@danielolsen danielolsen mentioned this pull request Jun 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenhagg jenhagg jenhagg approved these changes

Assignees

@danielolsen danielolsen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@danielolsen @jenhagg