test(LR18): add kg CO₂ range conversion integration test#1627
test(LR18): add kg CO₂ range conversion integration test#1627johnny603 wants to merge 5 commits into
Conversation
|
@johnny603 & @madisontran16 I'm assuming you will remove the Draft/WIP from this PR when it is ready for review. If you want me to look at it without doing that then please put in a comment. I just wanted to be clear that I'm not looking at the work at this point. If you need anything then please let me know. |
Hi, the LR18 changes are now ready for review, so we’ll be removing the Draft/WIP status from the PR shortly. This update ended up being a bit different from LR1–LR7 because LR18 validates unit conversion behavior in addition to aggregation behavior. Instead of only testing kWh → kWh responses, LR18 required seeding custom conversion fixtures and validating output in Main updates made:
A few questions while reviewing:
Thanks! |
|
@madisontran16 You are a contributor to this PR and the CLA box is checked but I did not see you in the OED records. Can you please either do the CLA or let me know if you think our records are off. |
huss
left a comment
There was a problem hiding this comment.
@johnny603 & @madisontran16 Thank you for this contribution to OED. Review and testing found it works as expected. I made one small comment within the code to consider. Also, I could not easily comment in this file so I am putting this comment here:
The directory (I think) OED is showing as a change in this PR. I'm not sure what this file is for but I think it should be removed from the PR.
As for your questions, this test does need different units/conversions than the other tests in the file. This is done in many of these test files and it is done the way you did it so your setup is fine. Specifically, the names in other tests may vary and that is fine. Also, since these tests are not as common, OED chose not to abstract the unit/conversion/meter definitions into a central. This was also done because they are often different between the tests.
Please let me know if anything is not clear or you have thoughts/questions.
| /* eslint-disable @typescript-eslint/no-var-requires, no-undef, @typescript-eslint/indent, comma-dangle, max-len */ | ||
|
|
There was a problem hiding this comment.
I'm not sure why the eslint-disable was added. I removed it and did not see any compile issues. Given this, I think these two new lines should be removed unless you feel otherwise.
There was a problem hiding this comment.
@johnny603 I see you gave this a thumbs up and resolved this comment. However, I don't see a change for this. Am I missing something? Do you think it should stay? I unresolved this comment for now.
I verified with Should be removed now, thanks for pointing that out |
Description
Followed design of other test cases in the
Part of #962 fix
Type of change
Adds a test case
Checklist
(Note what you have done by placing an "x" instead of the space in the [ ] so it becomes [x]. It is hoped you do all of them.)
Limitations
(Describe any issues that remain or work that should still be done.)
Contributors
@johnny603
@madisontran16