created a working test for CG12 | Group: "TeamExtreme"#1629
Conversation
|
@KJeansGR Thank you for this contribution. The PR description states the CLA has been signed by all contributors. However, the OED records do not have one for you. Furthermore, @WillyxWonka is the author of the one commit of this work. Thus, I would like you to:
Once this is done, this PR can be formally reviewed. Given the limitation listed in the PR description, I did take a very quick look at the code. Here are some thoughts:
Please let me know if anything is not clear, you have thoughts or need something. |
|
Very cool ! here is my best response to your concerns
Here is my best response to your concerns:
The package.json change was unrelated to CG12, and I will remove it from
the PR.
Regarding the direct insertion into cik: during debugging I found that the
compare readings path was returning null values unless the Electric_Utility
→ kg CO₂ conversion existed in cik. The conversion existed in the
conversions table and redoCik(conn) was being called, but the expected
conversion path was still not appearing in cik for the compare readings
query.
I added the direct cik insertion as a temporary debugging workaround to
verify that the compare group calculation itself produced the expected CG12
values once the conversion path existed.
Line quantity tests such as L18 and LG18 passed successfully using similar
conversion definitions, but compare group quantity tests continued
returning null values.
I agree that directly inserting into cik is probably not the correct final
approach. I will continue investigating whether this is:
-
an issue in my test setup
-
or a bug/inconsistency in the compare readings conversion pipeline.
I was hoping for more clarity because I initially attempted to use the
provided conversion definitions directly from the testing documentation,
but the compare group test consistently returned null values. I may be
misunderstanding part of the intended setup, so I plan to discuss this
further with my group as well. I moved quickly on the PR because I thought
I may have identified a real issue in the compare conversion path.
Regarding contributors: this work was completed as part of my class
capstone group assignment, but I am the only contributor to this specific
PR/code change. Also, the @WillyxWonka GitHub profile referenced in the
commit history is also me using a different username/account identity.
I will also complete the CLA requirements.
…On Sun, May 24, 2026 at 6:10 AM Steven Huss-Lederman < ***@***.***> wrote:
*huss* left a comment (OpenEnergyDashboard/OED#1629)
<#1629 (comment)>
@KJeansGR <https://github.com/KJeansGR> Thank you for this contribution.
The PR description states the CLA has been signed by all contributors.
However, the OED records do not have one for you. Furthermore,
@WillyxWonka <https://github.com/WillyxWonka> is the author of the one
commit of this work. Thus, I would like you to:
1. You, the other person and anyone else who worked on this to sign
the CLA or let me know that you think our records are off.
2. Add a comment that gives the names of any other people involved or
states that you are the only two people.
Once this is done, this PR can be formally reviewed.
Given the limitation listed in the PR description, I did take a very quick
look at the code. Here are some thoughts:
- You changed the package.json file. I'm unclear on why so would like
an explanation of why you wanted to do this. If this is a good idea, it is
likely it should be a separate PR as I don't think it is directly related
to the test case.
- Tests of readings should not be inserting into cik in a direct way.
This likely indicates either an issue with the test or a bug in OED.
Whatever the cause, it needs to be figured out. I'm unsure why you did this
so it is hard for me to comment too precisely on what might be wrong. I'm
also not doing a detailed analysis until the CLA is done.
Please let me know if anything is not clear, you have thoughts or need
something.
—
Reply to this email directly, view it on GitHub
<#1629 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BA3X3T3OT2VTHGFLBU33DHL44LYELAVCNFSM6AAAAACZK3XAS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKMRYG44DSNZRGE>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@WillyxWonka Thank you for the quick and complete comment. My thoughts are below.
Good. Don't forget to revert the package-lock.json.
First, let me say that your debugging of the issue was quite good. I think it may be best for you to not spend more time working on this until I can have a look as it is unexpected. I will try to find time soon to do that and let you know what I find. If you feel otherwise then let me know.
Sounds good and thanks for the clarification. |
|
Very cool, Have a nice day friend!
…On Sun, May 24, 2026 at 9:20 AM Steven Huss-Lederman < ***@***.***> wrote:
*huss* left a comment (OpenEnergyDashboard/OED#1629)
<#1629 (comment)>
@WillyxWonka <https://github.com/WillyxWonka> Thank you for the quick and
complete comment. My thoughts are below.
The package.json change was unrelated to CG12, and I will remove it from
the PR.
Good. Don't forget to revert the package-lock.json.
Regarding the direct insertion into cik: during debugging I found that the
compare readings path was returning null values unless the Electric_Utility
→ kg CO₂ conversion existed in cik. The conversion existed in the
conversions table and redoCik(conn) was being called, but the expected
conversion path was still not appearing in cik for the compare readings
query. I added the direct cik insertion as a temporary debugging workaround
to verify that the compare group calculation itself produced the expected
CG12 values once the conversion path existed. Line quantity tests such as
L18 and LG18 passed successfully using similar conversion definitions, but
compare group quantity tests continued returning null values. I agree that
directly inserting into cik is probably not the correct final approach. I
will continue investigating whether this is: - an issue in my test setup -
or a bug/inconsistency in the compare readings conversion pipeline. I was
hoping for more clarity because I initially attempted to use the provided
conversion definitions directly from the testing documentation, but the
compare group test consistently returned null values. I may be
misunderstanding part of the intended setup, so I plan to discuss this
further with my group as well. I moved quickly on the PR because I thought
I may have identified a real issue in the compare conversion path.
First, let me say that your debugging of the issue was quite good. I think
it may be best for you to not spend more time working on this until I can
have a look as it is unexpected. I will try to find time soon to do that
and let you know what I find. If you feel otherwise then let me know.
Regarding contributors: this work was completed as part of my class
capstone group assignment, but I am the only contributor to this specific
PR/code change. Also, the @WillyxWonka <https://github.com/WillyxWonka>
GitHub profile referenced in the commit history is also me using a
different username/account identity. I will also complete the CLA
requirements.
Sounds good and thanks for the clarification.
—
Reply to this email directly, view it on GitHub
<#1629 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BA3X3T2HEM7V7TVYQYQHUAL44MOMRAVCNFSM6AAAAACZK3XAS6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DKMRZGMYTGMBZG4>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
| }; | ||
|
|
||
| await prepareTest( | ||
| unitDatakWh.concat([u10, u12]), |
There was a problem hiding this comment.
@KJeansGR The testing design document specified u2, u10 & u12 but you don't have u2. Also, c11 & c12 but you don't have c12. I searched the code base for "kWh as kg of CO2" to find tests that also use these units. This revealed BG12 that uses these units and is a group test (but for bar not compare). (There are others but they differ more). It may help to look at this one to see how to set this up. I cannot be certain but I think this is what is causing your issues and the need to touch cik. The missing unit/conversion means the needed cik entries are not there. Please let me know if anything is not clear, you have thoughts/question or need help.
Also, don't forget the CLA & the package changes.
| @@ -333,7 +333,82 @@ mocha.describe('readings API', () => { | |||
| }); | |||
|
|
|||
| // Add CG12 here | |||
There was a problem hiding this comment.
Could you please remove this placeholder comment now that the test is done.
| }); | ||
|
|
||
| expectCompareToEqualExpected(res, expected, GROUP_ID); | ||
|
|
There was a problem hiding this comment.
Really small but could you remove these two blank lines.
4 participants
Description
Adds CG12 compare group quantity test coverage for kWh to kg CO₂ conversion behavior in readingsCompareGroupQuantity.js.
This test validates that compare readings for groups correctly return converted quantity values when using the kg CO₂ graphic unit.
Partly Addresses #[issue]
Type of change
(Check the ones that apply by placing an "x" instead of the space in the [ ] so it becomes [x])
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
The test currently inserts the required cik conversion row directly during setup to ensure the compare readings query has access to the expected conversion path.