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 conditional checks and suppress specific log warnings #761

Merged

Conversation

rh0dium
Copy link
Contributor

@rh0dium rh0dium commented Jan 7, 2025

Pull Request Description

Conditionally check results only when all expected data is available to ensure complete validation. Suppress log warnings related to temporary directory removal to avoid unnecessary test failures.

Checklist

Not all may apply:

  • OS-HPXML git subtree has been pulled
  • 301validator.xml has been updated (reference EPvalidator.xml)
  • Sample files have been added/updated (openstudio tasks.rb update_hpxmls)
  • Tests have been added/updated (e.g., rulesets\tests and/or workflow/tests/*_test.rb)
  • Documentation has been updated
  • Changelog has been updated
  • openstudio tasks.rb update_measures has been run
  • No unexpected changes to simulation results on CI

Conditionally check results only when all expected data is available to ensure complete validation. Suppress log warnings related to temporary directory removal to avoid unnecessary test failures.
@shorowit
Copy link
Collaborator

shorowit commented Jan 7, 2025

@rh0dium These changes look fine to me. Did you need to make any changes for other tests (like the HERS Method test) or is this good to go?

@rh0dium
Copy link
Contributor Author

rh0dium commented Jan 8, 2025

The other issue is that on test_RESNET_hers_iad_home_auto_generation and _test_RESNET_hers_reference_home_auto_generation you assume that 01-L100.xml is always passed in. I could update that for you in this as well if you'd like. Let me know.

@rh0dium
Copy link
Contributor Author

rh0dium commented Jan 8, 2025

Oh and one other tiny thing

Could we do this on the last line of minitest_helper.rb

Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new unless ENV['RM_INFO'] # spec-like progress

This allows us to use RubyMine (which includes the SpecReporter)

@shorowit
Copy link
Collaborator

shorowit commented Jan 8, 2025

Yep, feel free to make those changes.

Simplified and standardized the CSV generation to dynamically handle variable file inputs and their components. Adjusted indentation and added conditional logic to improve readability and maintainability throughout. Also ensured compatibility with specific environments in the minitest configuration.
@rh0dium
Copy link
Contributor Author

rh0dium commented Jan 8, 2025

Updated - Note: I did refactor a bit of the logic to make it clear in the headers which file relates to which output. I think this is probably preferred over the arbitrary "Test Results".

Copy link
Collaborator

@shorowit shorowit left a comment

Choose a reason for hiding this comment

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

LGTM! Ran the tests locally and they all passed.

@shorowit shorowit merged commit 6666021 into NREL:master Jan 8, 2025
@rh0dium rh0dium deleted the resnet-test-conditional-compare branch January 8, 2025 19:19
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