Added error handling in GetMeteredVariables so that invalid Meter:Customs don't cause a crash

[JasonGlazer]

Added a check so that invalid Meter:Custom do not cause a crash and instead show a warning message in the ERR file. This is related to #4631

[JasonGlazer]

I checked a bunch of the failed tests and they all seem to be EIO changes that are just changes to wording rather than changes to values. I am not sure why these were shown as changes to this pull request since I don't think I changed any wording of messages in the EIO files.

[JasonGlazer]

@mjwitte and @Myoldmopar Is it possible that the CI tests are picking up some other changes and showing them as diffs to these files?

[mjwitte]

@JasonGlazer It's because develop is ahead of your branch. If you want to clear those, you need to pull the latest develop and merge it into your branch, then push it back up.

[mjwitte]

@JasonGlazer Yesterday or so, I merged in a bunch of filed name changes that Rich did for unitary system and sizing. So, that's causing differences in lost of eio output lines.

[JasonGlazer]

Having the CI system compare against a different version than the source than was the basis for the changes to each fix means that each developer needs to figure out what is going on this time. Maybe the CI should change what the base case is.

[Myoldmopar]

I actually disagree. We tried to think about better options at one point, but ultimately this is the right approach. Here are my thoughts .

Pushing the big green merge button will take the current state of this branch and push it to the current state of develop. If this branch has deviated far off from this branch, the effects of the merge could cause develop to move too far. Even if there are no diffs from the original branch point, once the new code is merged with develop, the effect of the changed, merged state is indeterminate. By pulling develop into this branch, when you push the green button, develop simply "becomes" (fast-forwards) this branch, so if it is stable here, it will be stable there.

This approach encourages people to keep their branches more tightly up to date with develop. This helps with avoiding conflicts and also making sure new patterns are adopted. With Git, a quick merge from develop into a branch only takes a few seconds (barring conflicts that would have to be resolved anyway). That time and effort is well worth the confidence that what is going into develop is going to keep develop stable.

All of that said, I am certainly open to discussion about it or any other annoyances.

[mjwitte]

@Myoldmopar @JasonGlazer The risk here, which we've discussed before is that develop can creep incrementally (or significantly with one rogue merge) and we aren't doing any automated regressions back to the prior release. That step has to happen (as painful as it may be when we've had sweeping changes that cause massive small diffs) at some point before each release. My understanding is that this is manual for now.

[Myoldmopar]

It's certainly possible that all package builds (iteration-builds, release-builds, special-tagged-package-builds) could include a special regression back to the previous release. That test is manual right now, but we could think about that for future CI enhancements.

Something to think about, @kbenne @lefticus @lgu1234.

[mjwitte]

And if we are open to that, I would suggest that it include, or only include, annual simulations with diffs on daily or monthly results.

[JasonGlazer]

The current system does not provide the individual developer an understanding if failed tests mean the code is broken and because of this, all the developer can assert is that it should work or not. That seems to defeat one of the goals of CI.

Can we at least add an additional CI test suite on one platform that compares the new code to the code used as a basis for the new code? This would provide the developer an understanding that the code works or not.

[jasondegraw]

@JasonGlazer Does RVariableTypes need to get allocated before this point? The test is failing on the CI build, and this is the only thing I can find that looks likely.

[JasonGlazer]

@jasondegraw Thanks for finding this issue, I wasn't seeing any problem when I was running it locally on my machine. I hope if fixes the CI builds.

[jasondegraw]

@JasonGlazer Edwin @Myoldmopar was the one who actually caught it, so I shouldn't take too much credit. The issue never came up on any of the platforms we tried manually, even Ubuntu - just on the CI.

I added a couple of deallocations of the globals in the test and merged in develop. Once the CI comes back I think we are good to go.