Issue 1439 Fix by tazmeenahhmed · Pull Request #1588 · OpenEnergyDashboard/OED

tazmeenahhmed · 2026-03-27T01:18:04Z

Description

Implemented changes described in issue #1439 which are the following:

Removed function compareMeters from file csvPipelineTest.js and replaced all instances of compareMeters with an imported expectMetersToBeEquivalent function from meters.js
Added tests in meters.js that check if Meter.js functions getByIdentifier and getAll work properly
Rolled tests from meterTests.js into meters.js and deleted meterTests.js
Added tests in meters.js to check that Meter.js functions insert() and update() return correct values when Meter.convertUnitValue is called within them
Added a test in meters.js that checks if Meter.convertUnitValue returns -99 when passed with null
Added areaUnit and readingFrequency to meters.js function expectMetersToBeEquivalent to match mocha tests

Fixes #1439

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.

…match mocha tests

…when passed with null

…t values when Meter.convertUnitValue is called within them

…in meters test

…rted expectMetersToBeEquivalent from meters.js

huss

@tazmeenahhmed Thank you for another contribution and addressing this issue. Overall, the changes look good. I've made a few comments to consider. Please let me know if anything is not clear, you have thoughts or would like my help.

huss · 2026-03-29T15:59:59Z

 const sqlFile = database.sqlFile;

-class Meter {
+class Meter { 


There is a space added to the end of this line. OED prefers not to do that and this is the only change in the file. Could you undo this so no changes are shown in the file.

huss · 2026-03-29T16:22:32Z

 			expect(meter).to.have.property('previousEnd', null);
+			expect(meter).to.have.property('areaUnit', Unit.areaUnitType.METERS);
+			expect(meter).to.have.property('readingFrequency', null);
+


Please remove the blank line.

huss · 2026-03-29T16:35:15Z

 			expect(meter).to.have.property('previousEnd', '2020-03-05T13:15:13.000Z');
+			expect(meter).to.have.property('areaUnit', Unit.areaUnitType.METERS);
+			expect(meter).to.have.property('readingFrequency', 'PT13H57M19S');
 		} else {


src/server/models/Meter.js has these params that I don't see being checked:

* @param minVal inclusive minimum acceptable reading value (won't be rejected) * @param maxVal inclusive maximum acceptable reading value (won't be rejected) * @param minDate inclusive earliest acceptable date (won't be rejected) * @param maxDate inclusive latest acceptable date (won't be rejected) * @param maxError maximum number of errors to be reported, ignore the rest * @param disableChecks disable checks on meter for conditionSet

I think they are all admin only so null for non-admins.

huss · 2026-03-29T16:46:43Z

 });
+
+mocha.describe('Meter model', () => {
+	let unitId;


It seems only the last test uses the unitId set in the beforeEach. Given this, I'm wondering about moving this code to that test.

huss · 2026-03-29T19:05:25Z

+		}
+	});
+
+	mocha.it('can get meter by identifier', async () => {


I like this and the next new tests. Two thoughts about these and they may apply to other tests:

A few values are null/undefined. I wonder if some tests should use other values to verify that works.

The other values discussed in another comment that are part of a Meter do not have explicit values for the meter being created. That should be done for the tests.

huss · 2026-03-29T19:05:58Z

+				Unit.areaUnitType.METERS, undefined);
+
+			await Promise.all([meterA, meterB, meterC].map(meter => meter.insert(conn)));
+			const allExpectedMeters = await Meter.getAll(conn);


I think Expected and Actual should be the other way around. Normally expected is what the test knows is correct and actual is what you get back. Maybe this is based on another test but I would still like them flipped (if you agree).

huss · 2026-03-29T19:06:35Z

+			'MeterB', 'notes 2', 35.0, true, true, '01:01:25', '00:00:00', 5, 0, 1, 'increasing', false,
+			1.5, '0001-01-01 23:59:59', '2020-07-02 01:00:10', '2020-03-05 02:12:00', unitB.id, unitB.id,
+			Unit.areaUnitType.METERS, undefined);
+		const meterC = new Meter(undefined, 'Meter C', null, true, true, Meter.type.MAMAC, null);


This is a good test for the default value of -99 for the unit. It might be good to add another meter that explicitly sets the unit to -99 to verify that is the same.

huss · 2026-03-29T19:08:36Z

+				expectMetersToBeEquivalent(allExpectedMeters[i], allActualMeters[i]);
+			}
+	});
+});


Overall, the test coverage is good for all functions in Meter. I noticed two that are not tested and it would be good to do those so this is complete:

existsByName

makeMeterDataValid (do other one of this type)

Also, the pipeline tests do check about admin vs not (I think) but I think that having a test in this file that adds the info and gets as a non-admin to be sure the correct values are nulled out would be a good idea since it is directly related to the Meter interface.

tazmeenahhmed added 7 commits January 12, 2026 19:49

added areaUnit and readingFrequency to expectMetersToBeEquivalent to …

9ba11c7

…match mocha tests

Added a test to meters.js that checks if converUnitValue returns -99 …

cc26117

…when passed with null

added two tests to check that insert() and update() return the correc…

bbe9554

…t values when Meter.convertUnitValue is called within them

Rolled tests from meterTests.js into meters.js and removed meterTests.js

cb75be4

added a functions that checks getByIdentifier and getAll methods with…

282f389

…in meters test

Removed compareMeters frome csvPipelineTest and replaced with an impo…

f1b6a17

…rted expectMetersToBeEquivalent from meters.js

Merge branch 'OpenEnergyDashboard:development' into metertestcode

77b4f86

huss requested changes Mar 29, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1439 Fix#1588

Issue 1439 Fix#1588
tazmeenahhmed wants to merge 7 commits into
OpenEnergyDashboard:developmentfrom
tazmeenahhmed:metertestcode

tazmeenahhmed commented Mar 27, 2026

Uh oh!

huss left a comment

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

huss Mar 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

tazmeenahhmed commented Mar 27, 2026

Description

Type of change

Checklist

Uh oh!

huss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants