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.

Sign up for GitHub

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

Jump to bottom

Testing format changes #983

Merged
merged 11 commits into from
Sep 3, 2023
Merged

Testing format changes #983

merged 11 commits into from
Sep 3, 2023

Conversation

dzheng13341
Copy link
Contributor

@dzheng13341 dzheng13341 commented Aug 3, 2023 •
edited by huss
Loading

Description

  • Changed insertUnits() and insertConversions() so when adding inputs into test, it now matches with meterData instead of being arrays in an array, and also makes it easier to see/understand and change test data.
  • Subsequently changed all the current tests to match the new formatting.
  • Tests returned successful.

Partly addresses issue #940 because it does not do groups.

Type of change

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

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request

spearec
spearec approved these changes Aug 4, 2023
View reviewed changes
Copy link
Contributor

@spearec spearec left a comment

Choose a reason for hiding this comment

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

Changes look good. Both npm run test and npm run testData work fine, and I believe those are the only scripts impacted by these changes.
This certainly improves readability of this part of the code.

huss
huss requested changes Aug 4, 2023
View reviewed changes
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 @dzheng13341 for their first PR. The work is good. I made a couple of comments that I would like addressed. I note that the developer also checked that a cold start would work since these functions are used to initialize OED.

These are also used by the website setup so I will take care of updating that once this PR clears. That is not done within the production code of OED.

src/server/util/insertData.js Show resolved Hide resolved
src/server/util/insertData.js Outdated Show resolved Hide resolved
@dzheng13341
Added checks to make sure required keys
fd20ba2 
are provided for units and conversions.
Also changed sourceId and destinationId
to sourceName and destinationName.
huss
huss requested changes Aug 6, 2023
View reviewed changes
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.

I have a few comments and thoughts on the recent changes. Could you please address them before I do a careful check on the code? Thanks.

src/server/models/Conversion.js Outdated Show resolved Hide resolved
src/server/routes/conversions.js Outdated Show resolved Hide resolved
src/server/services/graph/createConversionArrays.js Outdated Show resolved Hide resolved
src/server/services/graph/createConversionGraph.js Outdated Show resolved Hide resolved
src/server/services/graph/handleSuffixUnits.js Outdated Show resolved Hide resolved
src/server/sql/conversion/update_conversion.sql Outdated Show resolved Hide resolved
src/server/test/db/conversionTests.js Outdated Show resolved Hide resolved
src/server/util/insertData.js Outdated Show resolved Hide resolved
src/server/util/insertData.js Outdated Show resolved Hide resolved
src/server/util/insertData.js Outdated Show resolved Hide resolved
dzheng13341 and others added 3 commits August 7, 2023 04:01
@dzheng13341
reverting some sourceName/destinationName to
89563b2 
sourceId/destinationId
@dzheng13341
revised code for insert conversions
daee046
@huss
units similar to conversions
3f7f684 
Update units to use similar checking for keys as conversions.
Added a couple of simple changes that I introduced into
the conversion code.
@huss huss mentioned this pull request Sep 3, 2023
Closed
@huss huss merged commit ad32738 into OpenEnergyDashboard:development Sep 3, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spearec spearec spearec approved these changes

@huss huss huss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dzheng13341 @huss @spearec