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

Final test C10 #1431

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

heleeon
Copy link

@heleeon heleeon commented Feb 22, 2025

Co-authored by: Caden Carpenter [email protected] @CadenC63
Co-authored by: Anthony Theng [email protected] @anthonytheng

Description

This pull request aims to allow testcase C10 to run alongside the other quantity meter tests.
It ensures that the Electric_Utility unit (BTU) is correctly correctly compared when read. The reading requested is in BTU.
This was accomplished through the specifications outlined in the DesignDocs

Partly addresses #962

(In general, OED likes to have at least one issue associated with each pull request. Replace [issue] with the OED GitHub issue number. In the preview you will see an issue description if you hover over that number. You can create one yourself before doing this pull request. This is where details are normally given on what is being addressed. Note you should not use the word "Fixes" if it does not completely address the issue since the issue would automatically be closed on merging the pull request. In that case use "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])

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

Limitations

No limitations we were made aware of.

Co-authored by: Caden Carpenter <[email protected]>
Co-authored by: Anthony Theng <[email protected]>
@Chocopepero
Copy link
Contributor

Hello, it appears you marked that the database is changed with this commit as well as the documentation needs an update with this merge. Can you confirm if this is the case?

Furthermore, can you add your group mates using their github usernames via @[username]?

I don't believe anyone has signed the CLA, please have everyone in the group do that.

For @huss, please edit the PR to change Fixes #[962] to Partially Addresses #962.

@heleeon
Copy link
Author

heleeon commented Feb 24, 2025

Hi, I don't believe our commit will change the database or require updated documentation.

I'll edit the PR to add their usernames and make sure my group signs the CLA.

@Chocopepero
Copy link
Contributor

All of the students have signed the CLA and everything in the code looks good.

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 @heleeon, @CadenC63 & @anthonytheng for their first contribution to OED. I have a few comment to address. Please let me know if anything is not clear.

mocha.it('C10: 1 day shift end 2022-10-31 17:00:00 for 15 minute reading intervals and quantity units & kWh as BTU', async () => {
// Use predefined unit and conversion data
const unitData = unitDatakWh.concat([
//adding u1, u2, u3, u16
Copy link
Member

Choose a reason for hiding this comment

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

u1 & u2 are defined in unitDataKWh and should not be redefined here. u3 & u16 should be concat here to define. The test case is failing on my machine and this is the reason why.

// Use predefined unit and conversion data
const unitData = unitDatakWh.concat([
//adding u1, u2, u3, u16
{ //u1
Copy link
Member

Choose a reason for hiding this comment

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

OED prefers not to have comments at the end of the line. Could you move these (this one will be removed but for later ones) to the next line above the code being described.

}
]);
const conversionData = conversionDatakWh.concat([
// adding c1, c2, c3
Copy link
Member

Choose a reason for hiding this comment

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

As with units, c1 is already defined and should not be added here.

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.

3 participants