Skip to content

Fixed failing EnergyPlus unit test on develop #6215#6216

Merged
Myoldmopar merged 8 commits intodevelopfrom
Failing-EnergyPlus-Unit-Test-On-Develop
Aug 7, 2017
Merged

Fixed failing EnergyPlus unit test on develop #6215#6216
Myoldmopar merged 8 commits intodevelopfrom
Failing-EnergyPlus-Unit-Test-On-Develop

Conversation

@Nigusse
Copy link
Copy Markdown
Contributor

@Nigusse Nigusse commented Jul 20, 2017 •
edited by mjwitte
Loading

Issue #6215

Pull request overview

Addressed failing unit tests on develop:

  1. AirTerminalSingleDuctMixer_SimPTAC_HeatingCoilTest
  2. ParallelPIUTest1
  3. VAVNoReheatTerminalUnitSchedule

The three unit test failure was caused by missing variables de-allocation. Now all EnergyPlus unit test suite run to completion in release mode in my Windows 7 machine. (from @Nigusse )

Also addressed failure in ZoneIdealLoadsTest.IdealLoads_PlenumTest. Added DataContaminantBalance::clear_state.

Work Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Unit Test(s)

@jasondegraw jasondegraw self-assigned this Jul 20, 2017
@jasondegraw jasondegraw added the Defect Includes code to repair a defect in EnergyPlus label Jul 20, 2017
@mjwitte mjwitte mentioned this pull request Jul 28, 2017
Merged
20 tasks
Myoldmopar
Myoldmopar requested changes Aug 7, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

This is great, I think we just need to add another clear_state to the unit test fixture. Can you confirm? If so I can throw it in there real quick and get this merged.

src/EnergyPlus/DataContaminantBalance.cc
ZoneContamGenericBLDiff.deallocate();
ZoneContamGenericDVS.deallocate();
ZoneContamGenericDRS.deallocate();
}
Copy link
Copy Markdown
Member

@Myoldmopar Myoldmopar Aug 7, 2017

Choose a reason for hiding this comment

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

This is great, thanks @Nigusse

src/EnergyPlus/PoweredInductionUnits.cc
{
PIU.deallocate();
}

Copy link
Copy Markdown
Member

@Myoldmopar Myoldmopar Aug 7, 2017

Choose a reason for hiding this comment

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

Not just one but two new clear_states. 👍 👍

tst/EnergyPlus/unit/Fixtures/EnergyPlusFixture.cc
DataBranchAirLoopPlant::clear_state();
DataAirSystems::clear_state();
DataBranchNodeConnections::clear_state();
DataContaminantBalance::clear_state();
Copy link
Copy Markdown
Member

@Myoldmopar Myoldmopar Aug 7, 2017

Choose a reason for hiding this comment

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

We need to add the PIU clear_state here too, right?

@jasondegraw jasondegraw removed their assignment Aug 7, 2017
@Myoldmopar
Copy link
Copy Markdown
Member

Myoldmopar commented Aug 7, 2017

Code changes look good. I went ahead and added in the second clear_state call. Built and ran the unit tests addressed and it's all fine. The tests weren't failing on my machine before, so I can't verify it, but CI sure seems happier. The Fin test failed and can be addressed next as it doesn't appear to be related to here. I'm merging so other PRs can take advantage of this fix. Thanks @Nigusse.

@Myoldmopar Myoldmopar merged commit 496fbd2 into develop Aug 7, 2017
@Myoldmopar Myoldmopar deleted the Failing-EnergyPlus-Unit-Test-On-Develop branch August 7, 2017 16:52
@Myoldmopar Myoldmopar mentioned this pull request Aug 7, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Myoldmopar Myoldmopar Myoldmopar approved these changes

Assignees

No one assigned

Labels

Defect Includes code to repair a defect in EnergyPlus

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Nigusse @Myoldmopar @nrel-bot @nrel-bot-2 @nrel-bot-3 @jasondegraw @mjwitte