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.

Sign up for GitHub

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

Jump to bottom

#94 - Add test for OutputDiagnostics, OutputDebuggingData, OutputJSON, and OutputTableSummaryReports #96

Merged
merged 1 commit into from
Mar 12, 2020
Merged

#94 - Add test for OutputDiagnostics, OutputDebuggingData, OutputJSON, and OutputTableSummaryReports #96

merged 1 commit into from
Mar 12, 2020

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Feb 26, 2020 •
edited
Loading

Pull request overview

Please change this line to a description of the pull request, with useful supporting information.

Link to relevant GitHub Issue(s) if appropriate:

This Pull Request is concerning:

  • Case 1 - NewTest: a new test for a new model API class,
  • Case 2 - TestFix: a fix for an existing test. The GitHub issue should be referenced in the PR description
  • Case 3 - NewTestForExisting: a new test for an already-existing model API class
  • Case 4 - Other: Something else, like maintenance of the repo, or just committing test results with a new OpenStudio version.

Case 1: New test for a new model API class

Please include which class(es) you are adding a test to specifically test for.

Work Checklist

The following has been checked to ensure compliance with the guidelines:

  • Tests pass either:

  • Ruby test is stable: when run multiple times on the same machine, it produces the same total site kBTU.
    Please paste the heatmap png generated after running the following commands:

    • I ensured that I assign systems/loads/etc in a repeatable manner (eg: if I assign stuff to thermalZones, I do model.getThermalZones.sort_by{|z| z.name.to_s}.each do ... so I am sure I put the same ZoneHVAC systems to the same zones regardless of their order)
    • I tested stability using process_results.py (see python process_results.py --help for usage).
      Please paste the text output or heatmap png generated after running the following commands: 
       $ python process_results.py test-stability clean
 $ python process_results.py test-stability run -n test_output_objects_rb --os_cli=$os_build/Products/openstudio
 $ python process_results.py test-status --tagged
 OK: No Failing tests were found

  $ python process_results.py heatmap --tagged
  OK: There are NO differences at all

Review Checklist

  • Code style (indentation, variable names, strip trailing spaces)
  • Functional code review (it has to work!)
  • Matching OSM test has been added or # TODO added to model_tests.rb
  • Appropriate out.osw have been committed
  • Test is stable
  • The appropriate labels have been added to this PR:
    • One of: NewTest, TestFix, NewTestForExisting, Other
    • If NewTest: add PendingOSM if needed

@jmarrec jmarrec added NewTest PR type: a new test for a new model API class PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release labels Feb 26, 2020
@jmarrec jmarrec self-assigned this Feb 26, 2020
jmarrec
jmarrec commented Feb 26, 2020
View reviewed changes
Copy link
Contributor Author

@jmarrec jmarrec left a comment

Choose a reason for hiding this comment

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

Added review comments as a code walkthrough.

model_tests.rb
Comment on lines +515 to +519
# TODO : To be added once the next official release
# including this object is out : 3.0.0
# def test_output_objects_osm
# result = sim_test('output_objects.osm')
# end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO for PendingOSM

model/simulationtests/output_objects.rb
Comment on lines +61 to +66
# NOTE: until https://github.com/NREL/EnergyPlus/issues/7742 is addressed
# You can only add maximum TWO diagnostics
output_diagnostics.setKeys(["DisplayExtraWarnings",
"ReportDuringWarmup",
# "DisplayAdvancedReportVariables"
])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this debugging object warrants making funny stuff in the FT right now to add several ouput:diagnostics objects with chunks of 2 keys...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbenne FYI, this one is worth noting.

@jmarrec jmarrec requested a review from kbenne March 10, 2020 16:14
@jmarrec jmarrec merged commit e4f4114 into develop Mar 12, 2020
@jmarrec jmarrec deleted the 94_OutputObjects branch March 12, 2020 13:16
jmarrec added a commit that referenced this pull request Apr 27, 2020
@jmarrec
Add OSM test with 3.0.0 official for #96 - Output Objects
3ee84a2
@jmarrec jmarrec added AddedOSM A matching OSM test has been added with an official OpenStudio release and removed PendingOSM A matching OSM test has yet to be added with the next official OpenStudio Release labels Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbenne kbenne Awaiting requested review from kbenne

Assignees

@jmarrec jmarrec

Labels
AddedOSM A matching OSM test has been added with an official OpenStudio release NewTest PR type: a new test for a new model API class
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add test for OutputDiagnostics, OutputDebuggingData, OutputJSON and OutputTableSummaryReports
1 participant
@jmarrec