Add unit tests for emcy.py (coverage improved from 73% → 99%) #604

yuvraj-kolkar17 · 2025-07-21T20:48:50Z

What type of PR is this?

/kind test
/area emcy.py

What this PR does / why we need it:

Adds a complete unit test suite for the emcy.py component under canopen/emcy.py.
This test ensures core EMCY functionalities work correctly and helps future development by validating key scenarios.

Which issue(s) this PR fixes:

Fixes #514

Does this PR introduce a user-facing change?

NONE

yuvraj-kolkar17 · 2025-07-25T19:01:05Z

Hi @acolomb
Just wanted to kindly follow up on this PR. Please let me know if there’s anything I need to improve or change. I'd love to contribute more to this project

acolomb · 2025-07-27T21:15:23Z

Will do as time permits.

codecov · 2025-07-27T21:16:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

yuvraj-kolkar17 · 2025-07-27T21:22:42Z

Will do as time permits.

Thanks for the update @acolomb ! I’ll wait for your review.
Does this project have an official Slack or community channel for contributors?

acolomb · 2025-07-27T21:25:26Z

Does this project have an official Slack or community channel for contributors?

None that I know of. Mainly GitHub for discussing code.

yuvraj-kolkar17 · 2025-07-27T21:28:54Z

Does this project have an official Slack or community channel for contributors?

None that I know of. Mainly GitHub for discussing code.

Got it, thanks for clarifying!

acolomb

Sorry for taking so long with the review. I started several attempts, but never got through to the end until now.

Some of these tests seem unnecessary IMO, see inline comments. And please make sure to follow a clean coding / formatting style.

acolomb · 2025-07-28T20:02:54Z

test/test_emcy.py

@@ -25,13 +26,11 @@ def check_error(self, err, code, reg, data, ts):
        self.assertAlmostEqual(err.timestamp, ts)

    def test_emcy_consumer_on_emcy(self):
-        # Make sure multiple callbacks receive the same information.


I'd rather see this converted to a docstring than removing.

acolomb · 2025-07-28T20:03:18Z

test/test_emcy.py

        acc1 = []
        acc2 = []
        self.emcy.add_callback(lambda err: acc1.append(err))
        self.emcy.add_callback(lambda err: acc2.append(err))

-        # Dispatch an EMCY datagram.


This is fine, no real added value in the comment.

acolomb · 2025-07-28T20:05:18Z

test/test_emcy.py

@@ -94,24 +91,20 @@ def timer(func):
            finally:
                t.join(TIMEOUT)

-        # Check unfiltered wait, on timeout.


I haven't check whack happens here, but it sounds like it may be harder to understand without the comment. So please try to leave other code untouched unless it's an obviously superfluous comment.

Same below a couple of times.

acolomb · 2025-07-28T20:06:53Z

test/test_emcy.py

@@ -123,6 +116,110 @@ def push_reset():
            t.start()
            self.assertIsNone(self.emcy.wait(0x9000, TIMEOUT))

+    def test_emcy_consumer_initialization(self):
+        """Test EmcyConsumer initialization state."""


Now this is pretty much redundant with the function name. 😉

Suggested change

"""Test EmcyConsumer initialization state."""

acolomb · 2025-07-28T20:08:25Z

test/test_emcy.py

+        self.assertEqual(consumer.log, [])
+        self.assertEqual(consumer.active, [])
+        self.assertEqual(consumer.callbacks, [])
+        self.assertIsInstance(consumer.emcy_received, threading.Condition)


I don't really get what important characteristics this function tests for. The emcy_received attribute being of a specific type seems to me like an implementation detail, not public, dependable API.

acolomb · 2025-08-05T09:20:36Z

test/test_emcy.py

+    def test_emcy_producer_network_assignment(self):
+        """Test EmcyProducer network assignment and usage."""
+        producer = canopen.emcy.EmcyProducer(0x100)
+        initial_network = producer.network
+
+        producer.network = self.net
+        self.assertEqual(producer.network, self.net)
+
+        producer.send(0x1000)
+        msg = self.rxbus.recv(TIMEOUT)
+        self.assertIsNotNone(msg)
+        self.assertEqual(msg.arbitration_id, 0x100)
+


I think this test is pretty redundant. We are using send and receive functions in several other tests, and this basically just asserts that assigning an attribute results in the value being assigned.

Suggested change

def test_emcy_producer_network_assignment(self):

"""Test EmcyProducer network assignment and usage."""

producer = canopen.emcy.EmcyProducer(0x100)

initial_network = producer.network

producer.network = self.net

self.assertEqual(producer.network, self.net)

producer.send(0x1000)

msg = self.rxbus.recv(TIMEOUT)

self.assertIsNotNone(msg)

self.assertEqual(msg.arbitration_id, 0x100)

acolomb · 2025-08-05T09:23:03Z

test/test_emcy.py

+    def test_emcy_producer_struct_packing(self):
+        """Test that the EMCY_STRUCT packing works correctly."""
+        from canopen.emcy import EMCY_STRUCT
+
+        packed = EMCY_STRUCT.pack(0x1234, 0x56, b'\xAB\xCD\xEF\x12\x34')
+        expected = b'\x34\x12\x56\xAB\xCD\xEF\x12\x34'
+        self.assertEqual(packed, expected)
+
+        code, register, data = EMCY_STRUCT.unpack(expected)
+        self.assertEqual(code, 0x1234)
+        self.assertEqual(register, 0x56)
+        self.assertEqual(data, b'\xAB\xCD\xEF\x12\x34')
+
+        packed = EMCY_STRUCT.pack(0x1234, 0x56, b'\xAB')
+        expected = b'\x34\x12\x56\xAB\x00\x00\x00\x00'
+        self.assertEqual(packed, expected)
+


This one is really testing an internal implementation detail, namely the structure used for decoding. That is already covered by most other tests. It could be solved differently, then this test might fail, although the public API to be tested is still functional.

Suggested change

def test_emcy_producer_struct_packing(self):

"""Test that the EMCY_STRUCT packing works correctly."""

from canopen.emcy import EMCY_STRUCT

packed = EMCY_STRUCT.pack(0x1234, 0x56, b'\xAB\xCD\xEF\x12\x34')

expected = b'\x34\x12\x56\xAB\xCD\xEF\x12\x34'

self.assertEqual(packed, expected)

code, register, data = EMCY_STRUCT.unpack(expected)

self.assertEqual(code, 0x1234)

self.assertEqual(register, 0x56)

self.assertEqual(data, b'\xAB\xCD\xEF\x12\x34')

packed = EMCY_STRUCT.pack(0x1234, 0x56, b'\xAB')

expected = b'\x34\x12\x56\xAB\x00\x00\x00\x00'

self.assertEqual(packed, expected)

acolomb · 2025-08-05T09:27:49Z

test/test_emcy.py

+        msg = self.rxbus.recv(TIMEOUT)
+        self.assertIsNotNone(msg)
+
+        self.consumer.on_emcy(msg.arbitration_id, msg.data, msg.timestamp)
+


Why not use the regular flow via network.subscribe()?

acolomb · 2025-08-05T09:28:00Z

test/test_emcy.py

+        msg = self.rxbus.recv(TIMEOUT)
+        self.assertIsNotNone(msg)
+
+        self.consumer.on_emcy(msg.arbitration_id, msg.data, msg.timestamp)
+


Why not use the regular flow via network.subscribe()?

acolomb · 2025-08-05T09:28:28Z

test/test_emcy.py


 if __name__ == "__main__":
    unittest.main()
+


Please watch out for accidental whitespace changes and inconsistencies.

Add test_emcy.py for EMCY module coverage

afa668f

yuvraj-kolkar17 mentioned this pull request Jul 21, 2025

Improve test coverage #514

Open

23 tasks

acolomb requested changes Aug 5, 2025

View reviewed changes

Add unit tests for emcy.py (coverage improved from 73% → 99%) #604

Are you sure you want to change the base?

Add unit tests for emcy.py (coverage improved from 73% → 99%) #604

Uh oh!

Conversation

yuvraj-kolkar17 commented Jul 21, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

Uh oh!

yuvraj-kolkar17 commented Jul 25, 2025

Uh oh!

acolomb commented Jul 27, 2025

Uh oh!

codecov bot commented Jul 27, 2025

Codecov Report

Uh oh!

yuvraj-kolkar17 commented Jul 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acolomb commented Jul 27, 2025

Uh oh!

yuvraj-kolkar17 commented Jul 27, 2025

Uh oh!

acolomb 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuvraj-kolkar17 commented Jul 27, 2025 •

edited

Loading