LR11: Add readings line meter range quantity test#1644
Conversation
Matterbranch
Removed comments
|
@matterwoven Thank you for this contribution. The PR description states the CLA has been signed by all contributors. However, OED records do not have one for you. Thus, I would like you to:
Once this is done, this PR can be formally reviewed. Please let me know if you have any questions/thoughts. |
|
The other contributors for this project are BushyMustache and DennysSaenz123 on GitHub, their signing at time of writing should be in the process of completion, I have also signed the form |
|
Update, currently contributor DennysSaenz123 is away and will be signing the document soon, but at a later point |
I have all the CLAs. I will try to review the PR soon. |
|
@matterwoven, @dennys123 & @BushyMustache Thank you for this contribution. I looked at it but there appears to be a number of unrelated files with changes. This is complicating the review. My first thought is that this branch is now slightly out of date with development. Since you are the developers, I wanted to see if you want to merge in the latest development. If not, let me know and OED can do it since it appears a clean merge. |
|
Thank you for letting me know, and apologies for the late response, me and my team have our own projects we're looking to so we would appreciate if OED could handle it, thanks again and appreciate the information |
@matterwoven, @dennys123 & @BushyMustache I'm sorry to report that merging development did not resolve all the issues. Given this I did not push my merge back to be safe. I have not tried to carefully check all the files but I already noticed these items:
Given this, I'm not going to review this PR further until the only changes are in the two expected test files. If you are unable to do that then this PR will be closed. (Only noting this since a previous comment indicated you may not have time.) I would appreciate knowing either way so we can properly track this PR. Please let me know if anything is not clear or you have thoughts/questions. |
matterwoven
left a comment
matterwoven
left a comment
There was a problem hiding this comment.
Cleaned up the issues you mentioned, if this helps the appeal let me know of any extra pieces needing changed, also note package-lock has been removed from this branch for re-creation, the old file seems unretrievable by me
Normally OED does not recreate the package-lock.json file but I will see if I can recreate. |
I put back the development version of package-lock.json and pushed out the change. The file is back and not showing changes in the PR. |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@matterwoven, @dennys123 & @BushyMustache Thank you for working on this. Review and testing found a few items to consider and I have made comments about them. I also want to note that LR11 is currently failing. This relates to PR #1659. The default tolerance is 10-7 but this test needs a value of 10-6. This PR will need to hold until that PR clears and the changed OED version is available to fix this situation. However, please make the other changes to prepare this PR. Please let me know if anything is not clear or you have thoughts/questions.
|
Thank you for the thorough follow up, I'll work in my spare time on preparing for this pull |
|
@matterwoven I recently merged PR #1659 that, if I recall correctly, you need for this PR. It deals with modifying the testing tolerance. If this is correct, you should add the latest development to your branch to do the needed work. It is a good idea to update anyway. Please let me know if you have any thoughts/questions. |
…o expected_line_range_ri_15_mu_kWh_gu_MJ_st_-inf_et_inf
|
Thank you for the merge! This addition to readingUtils.js was very useful, I added a 1 Delta defined deviation for tolerance and all tests pass, let me know if there's anything else I can do past this point, have an amazing day |
huss
left a comment
huss
left a comment
There was a problem hiding this comment.
@matterwoven Thank you for the updated code. It resolved all the open comments except one item. I think there was some confusion over the changes in expectRangeToEqualExpected as your updated code is still failing the test. Here is what I think needs to be done:
- It might be useful to do the step below to run your test before any changes to verify it fails on your machine.
- Fetch the OED (not your fork) development branch and merge it into your development branch on your machine. This should integrate the new code into your work. You can check by looking at the git commit records and/or the file with that code to see there are now 4 parameters for the function.
- Change the tolerance for the test to be 10-6. To do this change the call to the function to be
expectRangeToEqualExpected(res, expected, METER_ID, 0.000001);. - Follow the directions for testing your code. See the testing design doc (https://github.com/OpenEnergyDashboard/DesignDocs/blob/main/testing/testing.md#creating-a-new-test at step 9). It should show it as passing.
I'm hoping once this is done it will all work. Please let me know if you have any questions or something does not work as described.
2 participants
Description
Added LR11, additions fetch data in 15-minute interval readings with infinite start and end times. Test translates units from a document we added, converting the kWh format to MJ format with reverse conversion. (Document needed in Design Docs not present)
Addresses #LR11
Type of change
Test Document Alterations and Additions
Checklist
Limitations
A solution to the floating point error issue would be a true test file to replace our custom one. The testing file listed in the Design Docs was not locatable to the best of our ability, with full accuracy we will be able to clear tests without issues.