Skip to content
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

Added test BG14 #1352

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

Emily-D-2003
Copy link

@Emily-D-2003 Emily-D-2003 commented Oct 3, 2024

Description

Added tests BG14 from design document.

Partly addresses #962

Type of change

  • Note merging this changes the database configuration.
  • This change requires a documentation update

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.)

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request
  • You acknowledge that every person contributing to this work has signed the OED Contributing License Agreement and each author is listed in the Description section.

@Emily-D-2003 Emily-D-2003 mentioned this pull request Oct 3, 2024
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @Emily-D-2003 for their first pull request. First, my records do not indicate you signed the CLA. OED cannot accept your contribution without that. Second, I have made several comments to consider. I believe this is why the GitHub test failed for the new test case (it also failed locally on my machine).

src/server/test/web/readingsBarGroupQuantity.js Outdated Show resolved Hide resolved
@huss
Copy link
Member

huss commented Oct 20, 2024

@Emily-D-2003 Do you need any help with this pull request? I was checking the status of the pull requests and wondered about this one.

@Emily-D-2003
Copy link
Author

Emily-D-2003 commented Oct 24, 2024

@Emily-D-2003 Do you need any help with this pull request? I was checking the status of the pull requests and wondered about this one.

Thank you for the detailed comments and for checking in about the status of this PR! Just committed the updates made according to your comments.

One question - when attempting to sign the CLA, the webpage linked in the README returned a HTTP 404. Wondering if you're seeing the same?

Thanks!

@huss
Copy link
Member

huss commented Oct 24, 2024

One question - when attempting to sign the CLA, the webpage linked in the README returned a HTTP 404. Wondering if you're seeing the same?

Someone else just reported this and has a PR in to fix it in one place and I'll do the others. The correct link is https://openenergydashboard.org/developer/cla/. Thanks for asking.

@huss
Copy link
Member

huss commented Oct 24, 2024

There is some issue that I have not yet figured out. I plan to work more on this later. FYI for @Emily-D-2003 .

Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to @Emily-D-2003 for their changes. They addressed the comments already made but another item has now appeared. Please see my comment for details and options to address this. Note that testing the code locally (directions on the design docs test page) may help with working on this if you do that. Once that is addressed and the CLA is done then this should be fine.

];

//loads data into database
await prepareTest(unitDatakWh, conversionDatakWh, meterDatakWhGroups, groupDatakWh);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that the provided meters and groups have a default graphic unit of kWh but this test does not create that unit. This causes the code to fail. What needs to happen is the needed meters and groups must be created within this test. It is similar to the standard ones but modified somewhat. I created the needed code to test the solution. If you want that code or for me to commit/push the solution then let me know.

@huss
Copy link
Member

huss commented Dec 1, 2024

@Emily-D-2003 It has been over a month since the last comments where made on this PR and no changes have been made. I plan to close this PR in one week (about 12/8) if the updates are not made or no comment is made to indicate this is active.

@Emily-D-2003
Copy link
Author

@Emily-D-2003 It has been over a month since the last comments where made on this PR and no changes have been made. I plan to close this PR in one week (about 12/8) if the updates are not made or no comment is made to indicate this is active.

@huss Apologies for the delay! These comments slipped through somehow. On it, aiming to push the updates in a week. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants