Skip to content

Add unit tests for data models with factory methods - #3

Merged
rnovatorov merged 8 commits into
mainfrom
copilot/add-unit-tests-for-data-models
Feb 3, 2026
Merged

rnovatorov merged 8 commits into
mainfrom
copilot/add-unit-tests-for-data-models

Conversation

Copilot AI commented Feb 3, 2026 •

Copy link
Copy Markdown
Contributor
  • Explore repository structure and understand data models
  • Review existing test infrastructure
  • Install development dependencies
  • Create unit test directory structure for models
  • Write unit tests focused on factory methods only
  • Update Makefile to add test-unit target with coverage reporting
  • Run all tests to ensure they pass (13 tests passing)
  • Run linters to ensure code quality (black, isort, mypy, pyflakes all passing)
  • Address all PR review feedback:
    • Fixed imports to use from enapter_mcp_server.mcp import models
    • Removed ALL tests without actual logic to test (enums, models without factory methods, instantiation-only tests)
    • Kept only tests for factory methods (from_domain, from_dto, from_manifest)
    • Replaced mocks with real enapter.http.api dataclasses
    • Added coverage reporting with --cov --cov-report term-missing flags
    • Fixed enapter SDK imports to use import enapter (consistent with source files)
Original prompt

It would be nice to have some unit-tests for data models.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Copilot AI changed the title [WIP] Add unit tests for data models Add unit tests for data models Feb 3, 2026
Copilot AI requested a review from rnovatorov February 3, 2026 14:13
Comment thread tests/unit/mcp/models/test_alert_declaration.py Outdated
Comment thread tests/unit/mcp/models/test_alert_declaration.py Outdated
Comment thread Makefile Outdated
Copilot AI and others added 2 commits February 3, 2026 14:24
Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
- Changed imports from individual module imports to unified pattern: `from enapter_mcp_server.mcp import models`
- Removed 7 trivial "creation" tests that only verified basic object instantiation
- Updated Makefile to include coverage reporting in test-unit target
- All 65 tests pass with 100% coverage on model files

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Comment thread tests/unit/mcp/models/test_device_type.py Outdated
Comment thread tests/unit/mcp/models/test_alert_declaration.py Outdated
- Updated all test files to use `from enapter_mcp_server.mcp import models` pattern
- Removed all tests that only verify object instantiation without factory methods
- Kept only tests for factory methods (from_domain, from_dto, from_manifest) and behavior validation
- Test count: 48 tests (down from 65, removed 17 instantiation-only tests)

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread tests/unit/mcp/models/test_device_context.py Outdated
Comment thread tests/unit/mcp/models/test_alert_severity.py Outdated
- Deleted all enum test files (no logic in enums, just stdlib)
- Deleted DeviceContext and SiteContext tests (no logic to test)
- Deleted HistoricalTelemetry tests (no factory methods)
- Removed instantiation-only tests from remaining files
- Fixed import: changed `from unittest.mock import Mock` to `from unittest import mock`
- Kept only tests for factory methods (from_domain, from_dto, from_manifest)
- Test count: 13 tests (down from 48)

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Copilot AI changed the title Add unit tests for data models Add unit tests for data models with factory methods Feb 3, 2026
Copilot AI requested a review from rnovatorov February 3, 2026 15:24

@rnovatorov rnovatorov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's much better now, but some changes are still needed.

Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread Makefile Outdated
- Replaced unittest.mock.Mock with actual enapter.http.api dataclasses in tests
- Updated test_device.py to use enapter.http.api.devices.Device
- Updated test_site.py to use enapter.http.api.sites.Site
- Fixed Makefile test-unit target to include --cov --cov-report term-missing
- All 13 tests passing with 100% coverage on model factory methods

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Comment thread tests/unit/mcp/models/test_device.py Outdated
Comment thread tests/unit/mcp/models/test_site.py Outdated

@rnovatorov rnovatorov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixes of enapter SDK import statements required.

- Changed 'import enapter.http.api.devices' to 'import enapter' in test_device.py
- Changed 'import enapter.http.api.sites' to 'import enapter' in test_site.py
- Matches the import style used in source files
- All 13 tests passing

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
Copilot AI requested a review from rnovatorov February 3, 2026 16:15
@rnovatorov
rnovatorov marked this pull request as ready for review February 3, 2026 16:18

@rnovatorov rnovatorov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot, just one more thing. I see two approaches to testing results of factory method calls:

  1. Compare each attribute as you did. (assert alert.name == "low_pressure")
  2. Compare the resulting model objects (assert alert == models.Alert(...)).

Could you compare two approaches and describe their cons and pros and explain why you chose the current approach?

Copilot AI commented Feb 3, 2026

Copy link
Copy Markdown
Contributor Author

@copilot, just one more thing. I see two approaches to testing results of factory method calls:

  1. Compare each attribute as you did. (assert alert.name == "low_pressure")...

Comparison of Testing Approaches

Approach 1: Individual attribute assertions (current)

assert alert.name == "low_pressure"
assert alert.display_name == "Low Pressure Alert"
assert alert.severity == models.AlertSeverity.ERROR

Pros:

  • Clear test failure messages: Shows exactly which attribute failed
  • Tests the factory method's transformation logic explicitly
  • No dependency on __eq__ implementation
  • Easier to debug: pinpoints the specific field that's wrong

Cons:

  • More verbose
  • Requires updating if new attributes are added
  • Doesn't verify that ALL attributes were set correctly (could miss some)

Approach 2: Object comparison

assert alert == models.AlertDeclaration(
    name="low_pressure",
    display_name="Low Pressure Alert",
    severity=models.AlertSeverity.ERROR,
    ...
)

Pros:

  • More concise
  • Verifies the complete object state in one assertion
  • Less code to maintain

Cons:

  • Unclear failure messages: doesn't show which attribute differs
  • Depends on Pydantic's __eq__ implementation (though reliable)
  • Harder to debug: need to inspect the full object to find the difference
  • Still verbose due to needing to construct the expected object

Why I chose Approach 1:

For factory method tests, the primary goal is to verify that the transformation logic works correctly - i.e., that the factory method correctly maps from domain objects or DTOs to the model. Individual attribute assertions make it immediately clear which transformation failed, which is crucial for debugging. Since these models use Pydantic, __eq__ is reliable, but the clarity benefit of Approach 1 outweighs the conciseness of Approach 2 for these tests.

Copilot AI requested a review from rnovatorov February 3, 2026 16:24
@rnovatorov
rnovatorov merged commit 8399be4 into main Feb 3, 2026
1 check passed
@rnovatorov
rnovatorov deleted the copilot/add-unit-tests-for-data-models branch February 3, 2026 16:26
rnovatorov added a commit that referenced this pull request Mar 20, 2026
* Initial plan

* Add comprehensive unit tests for data models

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Refactor test imports and remove nonsense creation tests

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Refactor test imports and remove trivial creation tests

- Changed imports from individual module imports to unified pattern: `from enapter_mcp_server.mcp import models`
- Removed 7 trivial "creation" tests that only verified basic object instantiation
- Updated Makefile to include coverage reporting in test-unit target
- All 65 tests pass with 100% coverage on model files

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Fix imports and remove all instantiation tests

- Updated all test files to use `from enapter_mcp_server.mcp import models` pattern
- Removed all tests that only verify object instantiation without factory methods
- Kept only tests for factory methods (from_domain, from_dto, from_manifest) and behavior validation
- Test count: 48 tests (down from 65, removed 17 instantiation-only tests)

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Remove tests without actual logic to test

- Deleted all enum test files (no logic in enums, just stdlib)
- Deleted DeviceContext and SiteContext tests (no logic to test)
- Deleted HistoricalTelemetry tests (no factory methods)
- Removed instantiation-only tests from remaining files
- Fixed import: changed `from unittest.mock import Mock` to `from unittest import mock`
- Kept only tests for factory methods (from_domain, from_dto, from_manifest)
- Test count: 13 tests (down from 48)

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Use real enapter dataclasses instead of mocks and add coverage

- Replaced unittest.mock.Mock with actual enapter.http.api dataclasses in tests
- Updated test_device.py to use enapter.http.api.devices.Device
- Updated test_site.py to use enapter.http.api.sites.Site
- Fixed Makefile test-unit target to include --cov --cov-report term-missing
- All 13 tests passing with 100% coverage on model factory methods

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

* Use 'import enapter' instead of specific submodule imports

- Changed 'import enapter.http.api.devices' to 'import enapter' in test_device.py
- Changed 'import enapter.http.api.sites' to 'import enapter' in test_site.py
- Matches the import style used in source files
- All 13 tests passing

Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rnovatorov <20299819+rnovatorov@users.noreply.github.com>
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.

2 participants