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

Adding cloud-build artifacts to accelerate tests. #31

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

cboneti
Copy link
Member

@cboneti cboneti commented Nov 11, 2021

  • Dockerfile describes a container with everything needed to build and
    vet ghpc.
  • hpc-tolkit-builder corresponds to the cloud-build rule to build the
    builder and then run all tests.
  • hpc-toolkit-pr-validation uses the above container to validate a PR.

Submission Checklist:

  • Have you installed and run this change against pre-commit? pre-commit install
  • Are all tests passing? make tests
  • If applicable, have you written additional unit tests to cover this
    change?
  • Is unit test coverage still above 80%?
  • Have you updated any application documentation such as READMEs and user
    guides?
  • Have you followed the guidelines in our Contributing document?

* Dockerfile describes a container with everything needed to build and
vet ghpc.
* hpc-tolkit-builder corresponds to the cloud-build rule to build the
builder and then run all tests.
* hpc-toolkit-pr-validation uses the above container to validate a PR.
@cboneti
Copy link
Member Author

cboneti commented Nov 11, 2021

Just please notice I am explicitly skipping the go-unit-tests for the pre-commit since:

  • dnephin's go-unit-tests implementation sets a hard timeout of 30s for all tests and so they timeout in cloud build.
  • all the unit tests already run on the step zero of the build (make tests)

An alternative implementation, that I chose not to take was to change our .pre-commit-config.yaml to remove dnephin's go-unit-tests and add the following lines:

  • repo: git://github.com/Bahjat/pre-commit-golang
    rev: c3086ee
    hooks:
    • id: go-unit-tests

I tested it and it would then work. But since the tests run in step zero, it felt preferable to reduce the runtime avoiding duplicate runs AND preventing us to take yet another dependency.

Please comment if you don't think this is a good approach.

@cboneti cboneti self-assigned this Nov 11, 2021
Copy link
Contributor

@heyealex heyealex left a comment

Choose a reason for hiding this comment

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

This makes sense, I like it.

@cboneti cboneti merged commit af4b1dc into GoogleCloudPlatform:develop Nov 11, 2021
ek-nag added a commit to nagconsulting/hpc-toolkit that referenced this pull request May 2, 2024
…i-fixes

OFE fix: Partition and node placement options
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.

2 participants