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

Add tests to github actions CI #67

Merged
merged 44 commits into from
Jan 10, 2021
Merged

Add tests to github actions CI #67

merged 44 commits into from
Jan 10, 2021

Conversation

BenjaminRodenberg
Copy link
Member

@BenjaminRodenberg BenjaminRodenberg commented Nov 11, 2020

Closes #60

This PR adds tests for different building and installation options. This is especially useful for making sure that we do not break requirements for the spack image (such as the option --single-version-externally-managed).

Failing tests:

  • setup_install -> Will be fixed by updating dependencies on develop
  • setup_install_single_version_externally_managed -> Will be fixed via Clean-up of setup.py #65

@BenjaminRodenberg BenjaminRodenberg self-assigned this Nov 11, 2020
@BenjaminRodenberg
Copy link
Member Author

BenjaminRodenberg commented Nov 13, 2020

@fsimonis does the PR look more or less sane w.r.t usage of github actions? I will also open a PR in https://github.com/precice/ci-images in a moment. (edit: see precice/ci-images#1)

@fsimonis
Copy link
Member

Wouldn't it be a bit more resource-friendly to run all the installation versions only if the solverdummy test succeeds?

Some additional notes:

@BenjaminRodenberg
Copy link
Member Author

Tests finally working. Ready for review.

@fsimonis: Can you quickly check whether everything is "sane" from a github-actions-point-of-view?
@ajaust: Are we missing something important that we need for spack?

Copy link
Collaborator

@ajaust ajaust left a comment

Choose a reason for hiding this comment

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

I have added some comments. It is mainly about updating pip when it is installed via the system's package manager.

.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-test.yml Outdated Show resolved Hide resolved
@fsimonis fsimonis self-requested a review January 7, 2021 12:01
Copy link
Member

@fsimonis fsimonis left a comment

Choose a reason for hiding this comment

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

This PR looks good to me.
I suggest to merge it and see how it behaves on develop.
The action can always be tweaked if necessary.

One last idea: would it make sense to define dependencies between the tests to limit the amount of failing ones?

@BenjaminRodenberg
Copy link
Member Author

This PR looks good to me.
I suggest to merge it and see how it behaves on develop.
The action can always be tweaked if necessary.

One last idea: would it make sense to define dependencies between the tests to limit the amount of failing ones?

Yes and no. I like it that all tests are run in parallel and therefore finish after approx. 5 min (longest runtime: build_spack). A long sequential chain of tests will have a longer runtime. I think having "batches" of 2 depending jobs will be a good compromise (first the general one, then the more specific. First setup_install, then setup_install_single_version_externally_managed). I will implement this and then merge.

Trying to keep total runtime below 5mins
@BenjaminRodenberg BenjaminRodenberg merged commit ad14d44 into develop Jan 10, 2021
@BenjaminRodenberg BenjaminRodenberg deleted the i_60 branch January 10, 2021 19:30
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.

Use Github Actions CI
3 participants