-
Notifications
You must be signed in to change notification settings - Fork 22
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
#980 Expand scope of isProblem. #1046
Conversation
NOTE: This branch doesn't include unit tests as the branch is divergred from #1036 |
Test Results244 tests 235 ✅ 20s ⏱️ For more details on these failures, see this check. Results for commit d1a463e. ♻️ This comment has been updated with latest results. |
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.
Is this trying to fix the BUFR thing? looks a lot like it.
No, this is actually unrelated to MetPX/Sundew#20. This is me not having a proper testing infrastructure on DIRT and finding bugs later on while releasing code.. I have since corrected my testing infrastructure and this shouldn't happen again. |
Should leverage |
are you going to do that, or you want this merged as-is? |
I'll be doing that. I'll confirm once the PR is ready again. |
…r bug while fixing previous bug. Didn't cause any data loss.
Unit test failures unrelated to this PR. Ran gather/am tests locally and they all pass. Auditing the changes overnight to see if all looks well. |
…ion Mapping conditions, correct verifyHeader bug, add unit test coverage
sigh the unit tests are failing because I've modified a method recently and if I change it back now while testing it will cause this branches' code to fail. Will need to commit the correction after verifications are done |
yeah.. a challenge of unit tests is that they should be testing invariant, intended behaviour, and not stuff that just happens to be that way in the code as written. |
The PR should be ready now and good to go. However... idk why my changes to Anyways, a summary of what I've changed.
|
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.
does not affect flow tests.. all changes confined to AM which is tested by @andreleblanc11 who confirms they're good.
Passes new unit tests as well.
Found a bug with the
isProblem
added from the previous merge request for Sundew fixes.Also found another bug that wasn't affecting dissemination.