-
Notifications
You must be signed in to change notification settings - Fork 37
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
Wanghy/farm gamma validators #175
Wanghy/farm gamma validators #175
Conversation
Fixing issue with FARM validator test
…y-anl/HERON into wanghy/farmGammaValidators
The failed test shows the following failure for HERON:
|
@PaulTalbot-INL Paul, I see the issue. I haven't updated the submodule version of FARM yet, and this error should disappear after that change. Let me work with Congjian to get it fixed. I will keep you posted. Thanks, Haoyu |
Hey Paul, Congjian merged my raven PR idaholab/raven#1846 and the FARM submodule ID is updated. |
Job CentOS 8 on cd1b049 : invalidated by @dgarrett622 Restart due to submodule update |
It seems that this PR passed all the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment and a general question. We have a test for the Example
validator, should there also be a test or tests for FARM validators, or is that out of HERON scope? @PaulTalbot-INL @dylanjm
@wanghy-anl All HERON PRs need to be tied to an issue. I don't see an issue related to this PR, could you open an issue and link this PR to that issue? |
Once @dgarrett622 approves and merges this, it might be a couple days before we update the HERON submodule in RAVEN, just so we can get a couple other changes merged in as well. Will that cause any issue for you @wanghy-anl ? |
No worries Paul, that's not a pressing issue. Keeping the same HERON submodule ID will only cause the FARM validator tests to fail on raven test servers, which is allowed by raven. After updating the HERON submodule ID adopting the changes in this PR, these failed FARM validator tests will pass. |
Sure, and issue #176 is opened. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good. Once the tests pass this can be merged.
Pull Request Description
What issue does this change request address?
Closes #176
What are the significant changes in functionality due to this change request?
The FARM validators are removed from heron/src/validators, and a bridge was added in heron/src/validators/Factory.py
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.