Show additional pipeline values in logStatus output#1628
Conversation
desusaiteja
force-pushed
the
issue-1618-log-more-pipeline-values
branch
from
d6cffc0 to
9db5297
Compare
desusaiteja
force-pushed
the
issue-1618-log-more-pipeline-values
branch
from
9db5297 to
4761d72
Compare
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@desusaiteja Thank you for this contribution. Review and testing found it works as desired. I made a few comments to consider. One involves an oversight in the issue that will require more work. If you want to discuss then just let me know. If anything is not clear or you have questions/thoughts then just let me know.
| @@ -0,0 +1,11 @@ | |||
| Resolves #1618 | |||
There was a problem hiding this comment.
This seems to be a description of your work that is in the PR description. It should not be part of the OED repository and this file should be removed.
There was a problem hiding this comment.
Good catch. I removed oed-pr-body.md from the branch since that content belongs in the PR description, not the repository.
| @@ -0,0 +1,59 @@ | |||
| /* | |||
There was a problem hiding this comment.
I think this is a nice test of the work you did. My thoughts on it are:
- It does not test any other values in the log msgs so it isn't a general test of the code.
- OED has not tested logging across the code base so this is different than the rest of the code. That does not mean it is bad/wrong but if OED starts doing this then it should be continued across more of the code base. Given logging has not direct impact on the OED results, I have mixed feelings about doing this.
- A stronger test would be to make sure all the values passed to the function are actually shown so any new ones would not be missed in the future. That would be more complex but likely of greater value moving forward.
Given all of this, I am inclined to remove this test even though it is a nice usage of sinon. I welcome your thoughts.
Note if this is removed then the new export at the end of processDataTests.js can be removed.
There was a problem hiding this comment.
Thanks, that makes sense. Since the existing CSV pipeline tests cover the affected output, I removed the focused logging test and the temporary logStatus export.
| cumulativeReset, resetStart, resetEnd, readingGap, readingLengthVariation, onlyEndTime) { | ||
| cumulativeReset, resetStart, resetEnd, readingGap, readingLengthVariation, onlyEndTime, honorDst, | ||
| relaxedParsing, useMeterZone, warnOnCumulativeReset, useMeterFrequency, useMeterFrequencyVariation) { | ||
| let message = 'For reading #' + rowNum + ' on meter ' + meterName + ' in pipeline: ' + 'previous reading has value ' + prevReading.reading + ' start time ' |
There was a problem hiding this comment.
I ran the pipeline tests (npm run testsome src/server/test/web/csvPipelineTest.js) and 57 failed. I forgot when I created the issue that many of these tests check the returned information that is modified by this work. As a result, the new values need to be added to each of these tests. This is non-trivial work that will require looking at each failure message to see it is okay and copying the new values into the test code. Is this something you are willing to do? If not, someone else can do it but it needs to be done for this to merge.
Note I think this will effectively test what you added in processDataTests.js and I commented on it in that file. Given that, I am even more inclined to remove that test.
I also ran the full tests (npm run test) and no other tests failed due to your changes.
There was a problem hiding this comment.
Thanks for catching this. I updated csvPipelineTest.js so the expected pipeline log output includes the newly added status fields, including the honorDst cases. I also reran ./node_modules/.bin/mocha --timeout 15000 "src/server/test/web/csvPipelineTest.js", and it passes locally with 67 passing tests.
2 participants
Resolves #1618
Adds the newer pipeline flags to
logStatus()and updates each call site so they appear in the standardized logging output.Also adds a focused server-side test for the expanded log message. CLA submitted as per the guidelines.
Verification
node --check src/server/services/pipeline-in-progress/processData.jsnode --check src/server/test/db/processDataTests.js./node_modules/.bin/mocha --timeout 15000 "src/server/test/db/processDataTests.js"