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

Updated readingsBarMeterFlow.js for B17 test #1424

Merged
merged 3 commits into from
Feb 27, 2025

Conversation

albertfast
Copy link
Contributor

@albertfast albertfast commented Feb 14, 2025

Description

This pull request adds the B17 test case to the readingsBarMeterFlow.js file. This test validates the retrieval of 1-day bar data for 15-minute reading intervals, focusing on flow units (kW). The test verifies the system can correctly handle requests with an infinite start and end date, i.e., +-infinity.

#Purpose of the Test (B17):

The B17 test aims to ensure the Open Energy Dashboard correctly aggregates and presents energy consumption data as daily summaries (bars) for a meter that reports instantaneous flow rate (kW). In a real-world scenario, such a meter could be monitoring the real-time power consumption of an industrial machine, a building’s electrical system, or a renewable energy source like solar panels. This test confirms that the system can accurately calculate the total energy consumed over each day from this high-resolution flow rate data, even when the requested time period is unbounded.

#Data Processing Explanation:

The test leverages a pre-existing data file readings_ri_15_days_75.csv. This CSV contains simulated meter readings with these key features:

  • Reading frequency: 15-minute intervals (ri_15).

  • Duration: Data spans 75 days (days_75).

  • Data Range: Between 1 and 100.

  • Meter Details: The data represents the type of Electric (kW), by the following unit and the conversion:

    • u4: This unit is kW (kilowatts), representing real-time flow. 
      
    • u5: This unit is Electric, which is the base meter type that holds the flow value.
      
    • c4: Conversion where Electric->kW.
      
  • Data Selection: The test uses all of these readings in the bar graph, indicated by range L5:N79, to test an unbounded range of time.

The test case then checks that the values from expected_bar_ri_15_mu_kW_gu_kW_st_-inf_et_inf_bd_1.csv are the same as the aggregated daily rates, which tests the 15-minute rates are used for a period to generate one daily rate.

Configuration and CSV Creation:

I used the expected CSV spreadsheet to precompute the result.

To ensure accurate data retrieval and conversion, I ran the test with these settings:

  • Electric: u4, u5
  • Conversion to kW: c4
  • L5:N79 for the time range in the table.

This was the correct setup to test the code in its isolated environment.

Summary of Steps

Step Command Description
1. Start PostgreSQL 'docker run --name postgres-db -e POSTGRES_PASSWORD=password -d -p 5432:5432 postgres:15' Runs PostgreSQL inside a Docker container.
2. Connect to PostgreSQL 'docker exec -it postgres-db psql -U postgres' Opens PostgreSQL interactive shell.
3. Create Database 'CREATE DATABASE "oed-database";' Creates the required database.
4. Install Dependencies 'npm install --save sequelize sequelize-cli pg pg-hstore' Installs Sequelize and PostgreSQL libraries.
5. Initialize Sequelize 'npx sequelize init' Creates necessary Sequelize config files.
6. Run Migrations & Seeding 'npx sequelize db:migrate && npx sequelize db:seed:all' Sets up tables and inserts initial data.
7. Run Mocha Tests 'npx mocha /workspace/src/server/test/web/readingsBarMeterFlow.js --timeout 10000' Runs the test with a timeout.

Note: This guide ensures that every time the Codespace starts, you can quickly set up, migrate the database, seed data, and run tests without issues. 🚀

Partly addresses #962

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

Addressing Workspace Setup and Database Configuration Issues:

During the test setup, I encountered configuration issues related to the PostgreSQL database connection within the Codespaces Docker environment. I received an error when using code spaces. Specifically, authentication errors prevented the test from completing seamlessly within the system setup. Adding to the surprise, the CSV file required for this test wasn’t located in the expected src/server/test/web/readingsData/ directory. It was located in the DesignDocs repository instead, which was unexpected. Initially, I tried following the standard procedure to set up a workspace, starting up the Docker container, and logging in as described in the documentation. However, when I logged into the database using that method, the local file system was displayed instead of the repository to be tested.

// Add B17 here
mocha.it('B17: should have daily points for 15 minute reading intervals and flow units with +-inf start/end time & kW as kW', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The test description does not match what is on the testing document.

https://github.com/OpenEnergyDashboard/DesignDocs/blob/main/testing/testing.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the test description "B17: should have daily points for 15 minute reading intervals and flow units with +-inf start/end time & kW as kW for Electric meter"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Marked B17 as "X" in testing.md and created a pull request for this update.

@Chocopepero
Copy link
Contributor

@albertfast None of the checklist items have been marked as completed. Can you confirm that these were completed? Thank you.

@huss
Copy link
Member

huss commented Feb 14, 2025

During the test setup, I encountered configuration issues related to the PostgreSQL database connection within the Codespaces Docker environment.

I thought there was info with a warning about Codespaces but it is not there. I will track down why not and fix.

@huss
Copy link
Member

huss commented Feb 14, 2025

Adding to the surprise, the CSV file required for this test wasn’t located in the expected src/server/test/web/readingsData/ directory.

In the section on creating a test in the testing design doc, step 7 it has: In general, you need to copy the file from the design document testing/readingsData/ directory to the src/server/test/web/readingsData/ directory since the expected files are not included in the OED code until a test is written. If you think this is not clear or there was an issue about it then please let us know.

@huss
Copy link
Member

huss commented Feb 14, 2025

@albertfast Thank you for your work on OED. First, the box for completing the CLA in the description is checked but OED does not have a record that you did this. Can you either do it or let us know if you think our records are off. Second, I wanted to know if you worked on this alone or others were involved. If so, OED would like to know to verify their CLA agreement.

@albertfast
Copy link
Contributor Author

@Chocopepero I realized that my initial response was a bit brief. I have already updated the description, but after making the changes, I found alternative solutions, so I will make another update accordingly.

@huss It took me some time to realize that the CSV file was missing in the src/server/test/web/readingsData directory within OED. Once I noticed it, I uploaded it to my branch. Finally, I successfully ran the application using Docker and PostgreSQL and completed the test. My teammates and I worked together, and I finalized the last part. I hope it passes your tests as well.

@aizenbaidya and @khang Tagging you both since we worked on this together.

Best regards.

@huss
Copy link
Member

huss commented Feb 17, 2025

Thank you for your updates. OED is still waiting for a response from @albertfast &@khang on the status of their CLA. This is needed to move this PR forward. Thanks.

@albertfast
Copy link
Contributor Author

Thank you for your updates. OED is still waiting for a response from @albertfast &@khang on the status of their CLA. This is needed to move this PR forward. Thanks.

@huss

I have now completed the necessary steps for the B17 test:

✅ Marked B17 as "X" in testing.md and created a pull request for this update.
✅ Checked the required checkboxes in the PR form.
✅ I have added the steps I followed to run the test in the Description section.
✅ Verified the B17 test in readingsBarMeterFlow.js, ensuring the necessary CSV files were correctly processed and API calls worked as expected.
✅ I also updated the commit with co-authors:
@aizenbaidya and Khang Nguyen

Please review the PR and let me know if any further changes are needed. Thanks!

Sincerely,

@huss
Copy link
Member

huss commented Feb 24, 2025

Thanks to @albertfast for the recent comment and work. As indicated in a previous comment, OED needs to know that the CLA is signed by each person contributing. One comment lists khang and another Khang Nguyen. I'm unsure which GitHub user is Khang Nguyen but it does not seem to be khang. This is making it hard to verify if the CLA is signed by them but I cannot find any record that seems to match such a user. Thus, can you please:

  1. Clarify which GitHub ID is associated with this user along with the actual name.
  2. Make sure that this person signs (or has signed) the OED CLA.

@huss
Copy link
Member

huss commented Feb 24, 2025

The recent commit added changes to a handful of files that are not part of the test case being updated. I'm unclear on why the changes were made but it seems likely they need to be removed as unrelated to this PR. Please let me know if there was a reason for these.

Co-authored-by: Aizen Baidya <[email protected]>
Co-authored-by: Khang Nguyen <[email protected]>
I added the missing CSV file from DesignDocs/testing/readingsData/expected_bar_ri_15_mu_kW_gu_kW_st_-inf_et_inf_bd_1.csv
@albertfast
Copy link
Contributor Author

The recent commit added changes to a handful of files that are not part of the test case being updated. I'm unclear on why the changes were made but it seems likely they need to be removed as unrelated to this PR. Please let me know if there was a reason for these.

@huss
I initially made changes to config.json while troubleshooting PostgreSQL and Docker issues, but later realized they were unnecessary. To fix this, I reset my branch with:
git fetch origin
git reset --hard origin/development

Then, I re-added only the B17 test changes in readingsBarMeterFlow.js and committed them. I also restored the missing CSV file from DesignDocs/testing/readingsData to OED/src/server/test/web/readingsData.

Now, the B17 test is correctly updated and runs successfully in my Codespace. Let me know if you have any concerns.

Thanks!

@huss
Copy link
Member

huss commented Feb 24, 2025

@albertfast I appreciate your efforts on this work. Normally OED does not review pull requests until everyone OED believes is involved has agreed to the CLA. Thus, until the CLA comments are resolved, OED will wait to review the code any further. Please let us know if you have any thoughts.

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.

Thank you to @albertfest, @aizenbaidya & @khang for their first contribution and efforts on this PR. I have made one comment to be addressed.


});

mocha.it('B17: should have daily points for 15 minute reading intervals and flow units with +-inf start/end time & kW as kW for Electric meter', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I see where you have a comment stating it updated the description but I still don't see it matching the design doc of 1 day bars for 15 minute reading intervals and flow units with +-inf start/end time & kW as kW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @huss, I have updated the description to match the design document with: 'B17: 1 day bars for 15 minute reading intervals and flow units with +-inf start/end time & kW as kW' Please let me know if there are any further adjustments needed. Thanks!

Co-authored-by: Aizen Baidya <[email protected]>
Co-authored-by: Khang Nguyen <[email protected]>
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.

All comments have been addressed and everything works as expected. Congratulations to @albertfest, @aizenbaidya & @khang on their first accepted work for OED.

@huss huss merged commit 32f0bcb into OpenEnergyDashboard:development Feb 27, 2025
3 checks passed
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