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

Hariharan/cluster test #120

Merged

Conversation

hariharan-devarajan
Copy link
Collaborator

@hariharan-devarajan hariharan-devarajan commented Feb 9, 2021

Changes to ensure test can run on clustered env.

  • file creation for sample data for testing was not working on OrangeFS. First creating it locally on the node and then copying the file.
  • Fixed ambiguous abs definitions which occur on 9.3.0 and above.
  • Created a better docker file to use ubuntu:18.04 instead of gitpod.
  • added HERMES_INSTALL_TESTS as the option to install Hermes tests into the bin directory.
  • added spack package for Hermes with install instructions.
  • created ares-conf for hermes_stdio case.

@coveralls
Copy link

coveralls commented Feb 9, 2021

Pull Request Test Coverage Report for Build 552378203

  • 12 of 13 (92.31%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 80.668%

Changes Missing Coverage Covered Lines Changed/Added Lines %
adapter/src/hermes/adapter/stdio/stdio.cc 12 13 92.31%
Totals Coverage Status
Change from base Build 548937317: -0.7%
Covered Lines: 4803
Relevant Lines: 5954

💛 - Coveralls

@hariharan-devarajan hariharan-devarajan marked this pull request as ready for review February 9, 2021 00:43
Dockerfile Outdated Show resolved Hide resolved
Comment on lines 538 to 539
size_t offset = abs((int)(((i * rand_r(&info.offset_seed))
% info.stride_size) % info.total_size));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You make the same change to the same code in 4 different places in this file. Why not make a helper function size_t GetRandomOffset(size_t i, Info info); so you have this code in one place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

Choose a reason for hiding this comment

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

You define GetRandomOffset four times, which defeats the purpose. Can't you define it just once, pass the seed and stride size as parameters, and use the same function everywhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created a utility header file where I moved the function and called it from there.

@ChristopherHogan
Copy link
Collaborator

I'd prefer if you didn't add new unrelated commits to PRs that are already in the review process.

@hariharan-devarajan
Copy link
Collaborator Author

I'd prefer if you didn't add new unrelated commits to PRs that are already in the review process.

The problem is as I have coverage enabled its failing as coverage is reducing. I was fixing that. If you prefer, I can do it on a new PR to improve coverage.

@ChristopherHogan
Copy link
Collaborator

I'm not worried by the failing coverage. I understand that things are in flux right now. You can leave the added coverage commits here, but if you want to increase coverage further, please create a PR specifically for that purpose.

@hariharan-devarajan
Copy link
Collaborator Author

I'm not worried by the failing coverage. I understand that things are in flux right now. You can leave the added coverage commits here, but if you want to increase coverage further, please create a PR specifically for that purpose.

Sure. I wasn't sure about it. Thanks for clarifying.

@ChristopherHogan ChristopherHogan merged commit 200cbde into HDFGroup:master Feb 9, 2021
hariharan-devarajan added a commit to hariharan-devarajan/hermes that referenced this pull request Feb 9, 2021
Merge pull request HDFGroup#120 from hariharan-devarajan/hariharan/cluster_test
@hariharan-devarajan hariharan-devarajan deleted the hariharan/cluster_test branch March 4, 2021 16:38
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.

None yet

4 participants