Add aggregation unit tests #241
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #241 +/- ##
==========================================
- Coverage 27.26% 27.24% -0.02%
==========================================
Files 35 35
Lines 5465 5468 +3
Branches 771 772 +1
==========================================
Hits 1490 1490
- Misses 3898 3901 +3
Partials 77 77
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Nexarian
force-pushed
the
add_aggregates_unit_tests
branch
2 times, most recently
from
e6a9660 to
f6d4438
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds two unit tests for the /aggregates endpoint to validate basic functionality and negative solar power adjustment behavior.
- Adds
TestDoGetAggregatesEndpointstest class with two test cases - Tests basic
/aggregatesendpoint functionality and negative solar power handling - Imports
HTTPStatusfrom thehttpmodule for clearer status code assertions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_aggregates_with_negative_solar_adjustment(self, proxystats_lock, mock_safe_call, mock_pw): | ||
| """Test aggregates with negative solar power adjustment""" | ||
| self.handler.path = "/aggregates" | ||
| mock_pw.poll = Mock() | ||
| mock_safe_call.return_value = json.dumps({ | ||
| "solar": {"instant_power": -500}, | ||
| "load": {"instant_power": 2000} | ||
| }) | ||
|
|
||
| self.handler.do_GET() | ||
|
|
||
| self.assertEqual(proxy.server.proxystats["gets"], 1) | ||
| proxystats_lock.__enter__.assert_called_once() | ||
| proxystats_lock.__exit__.assert_called_once() | ||
| result = self.get_written_json() | ||
| self.assertEqual(result["solar"]["instant_power"], 0) |
There was a problem hiding this comment.
This test doesn't verify the load adjustment behavior. According to the server code (line 975 in server.py), when solar power is negative and neg_solar is False, the code attempts to shift energy from solar to load by subtracting solar["instant_power"] from load["instant_power"]. However, due to a bug in the server code (solar["instant_power"] is already set to 0 before the subtraction), the load value remains unchanged. This test should verify the load value to document the current behavior: self.assertEqual(result["load"]["instant_power"], 2000).
There was a problem hiding this comment.
Hmm, if this is truly a bug, we should fix it. I've done so, though I don't know if this is the intended behavior? What is "negative solar" supposed to mean?
Anyway, I've updated it to do what I think it's supposed to do and updated the unit test as well.
proxy/tests/test_do_get.py
Outdated
| """Test /aggregates endpoint""" | ||
| self.handler.path = "/aggregates" | ||
| mock_pw.poll = Mock() | ||
| mock_safe_call.return_value = {"solar": {"instant_power": 1000}} |
There was a problem hiding this comment.
The mock return value is inconsistent with the second test. In the second test (line 116), mock_safe_call.return_value returns a JSON string, but here it returns a dictionary. While both are handled by the server code (lines 963-967 in server.py), the tests should be consistent. Consider using the same type for both tests, preferably a dictionary since that's what safe_endpoint_call typically returns when jsonformat=True (the default).
proxy/tests/test_do_get.py
Outdated
| def test_aggregates_endpoint(self, proxystats_lock, mock_safe_call, mock_pw): | ||
| """Test /aggregates endpoint""" | ||
| self.handler.path = "/aggregates" | ||
| mock_pw.poll = Mock() | ||
| mock_safe_call.return_value = {"solar": {"instant_power": 1000}} | ||
|
|
||
| self.handler.do_GET() | ||
|
|
||
| mock_safe_call.assert_called_once_with( | ||
| "/aggregates", mock_pw.poll, "/api/meters/aggregates" | ||
| ) | ||
| self.assertEqual(proxy.server.proxystats["gets"], 1) | ||
| proxystats_lock.__enter__.assert_called_once() | ||
| proxystats_lock.__exit__.assert_called_once() | ||
| self.handler.send_response.assert_called_with(HTTPStatus.OK) |
There was a problem hiding this comment.
This test lacks assertions about the actual response content. While it verifies the mock was called correctly and stats were updated, it doesn't verify what data was written to the response. Consider adding an assertion like result = self.get_written_json() followed by self.assertEqual(result["solar"]["instant_power"], 1000) to verify the endpoint returns the expected data.
Nexarian
force-pushed
the
add_aggregates_unit_tests
branch
from
f6d4438 to
a60dd1f
Compare
|
@jasonacox Updated. |
2 participants
Adding two more tests that validate simple use-cases for the /aggregates endpoint.