feat(API): Add properties available in NI-XNET 18.0. #263

d-bohls · 2018-08-20T21:56:09Z

This contribution adheres to CONTRIBUTING.md.
New tests have been created for any new features or regression tests for bugfixes.
tox successfully runs, including unit tests and style checks (see CONTRIBUTING.md).

This change adds the new properties listed in #255.
This change does not address the two new methods for
converting byte arrays.

Add a new enum value:

DevForm.PCIE

Add new error codes:

NX_ERR_LOW_LEVEL_COMMUNICATION_TIMEOUT
NX_ERR_CDAQ_TRANSFER_ABORTED
NX_ERR_DATABASE_IMPORT_VERSION
NX_ERR_EVENT_UNEQUAL_PAYLOAD_LENGTH
NX_ERR_EVENT_UNEQUAL_CHECKSUM_TYPE
NX_ERR_CRIO_BAD_DRIVER_VERSIONS
NX_ERR_CRIO_MISSING_SLOT_SUPPORT
NX_ERR_INTF_ALREADY_IN_USE_BY_NI_XNET
NX_ERR_INTF_ALREADY_IN_USE_BY_NI_XCL
NX_ERR_BYTE_ARRAY_NOT_ALLOWED
NX_ERR_NO_BYTE_ARRAY_MIX
NX_ERR_ONLY_ONE_BYTE_ARRAY

Sort existing errors (matches changes made in nixnet.h):

NX_ERR_SESSION_TYPE_FRAME_INCOMPATIBILITY
NX_ERR_DIAGNOSTIC_SCHEDULE_NOT_DEFINED

Add two new driver properties, along with unit tests for them:

Interface.lin_checksum_to_in_strm
J1939.include_dest_addr_in_pgn

Here is the equivalent C API documentation for the new properties:

coveralls · 2018-08-20T22:00:36Z

Coverage decreased (-5.2%) to 62.549% when pulling 0d2d8c2 on d-bohls:add_18_0_props into efe3f4e on ni:master.

coveralls · 2018-08-20T22:00:36Z

Coverage increased (+0.03%) to 67.742% when pulling 1fd7732 on d-bohls:add_18_0_props into 053a519 on ni:master.

epage · 2018-08-20T22:01:03Z

nixnet/_cconsts.py

@@ -105,7 +105,6 @@
 NX_ERR_FRAME_SIZE_MISMATCH = (NX_ERROR_BASE | 0x0AA)
 NX_ERR_INDEX_TOO_BIG = (NX_ERROR_BASE | 0x0AB)
 NX_ERR_SESSION_MODE_INCOMPATIBILITY = (NX_ERROR_BASE | 0x0AC)
-NX_ERR_SESSION_TYPE_FRAME_INCOMPATIBILITY = (NX_ERROR_BASE | 0x15D)


These were moved and not removed

Yup, a change to nixnet.h sorted them (by error code) and so I did too, as mentioned in the commit.

epage · 2018-08-20T22:02:48Z

tests/test_j1939.py

+@mock.patch('nixnet._cfuncs.lib', MockXnetLibrary)
+def test_j1939_properties():
+    # This test should be expanded to cover more J1939 properties.
+    j1939.J1939.include_dest_addr_in_pgn = j1939.J1939.include_dest_addr_in_pgn


My main concern with this PR is the testing strategy.

This calls the getter and setter on the upper layers but ends in a dead end at the _cfuncs.XnetLibrary layer. What value does this offer us over mypy's checks?

For methods/properties which are simply thinly layered wrappers to a library call, I've questioned the value of using unit tests over integration tests. I've advocated that we test them all in integration tests since you get the benefit of making sure that (a) the driver can correctly handle the arguments from python and that (b) python can correctly handle an actual return value from the driver. I'm not saying that integration tests are always better, as that depends on a number of factors of course. I was given guidance that, for properties and methods, there should be some balance between integration tests and unit tests. It would be nice going forward if we had some documented testing guidelines that address these issues.

For properties, in general, it makes sense to write integrations tests over unit tests. That being said I don't expect us to write an integration test for each property in the API. Most commonly or frequently used properties in that class should suffice. I'd suggest checking with the support engineer or the rest of the XNET team to learn about which properties might be more common or important. For functions or methods, we've been doing a mix of unit and integration tests where applicable. If there's business logic in the python layer or if it's not a simple pass through into the C layer, we've been writing unit tests and I'm hoping we can continue to do that.

I realize "most commonly or frequently" used APIs can be a bit arbitrary and differ based on opinion but I think that's what we've been doing so far. I'm open to other ideas or suggestions on how we should go about deciding when to write unit vs integration or how many integration tests to write, etc. Please let me know if you have ideas or thoughts on this.

jashnani

Looks fine overall. I have some minor comments.

jashnani · 2018-10-08T21:54:10Z

tests/test_j1939.py

+@mock.patch('nixnet._cfuncs.lib', MockXnetLibrary)
+def test_j1939_properties():
+    # This test should be expanded to cover more J1939 properties.
+    j1939.J1939.include_dest_addr_in_pgn = j1939.J1939.include_dest_addr_in_pgn


For properties, in general, it makes sense to write integrations tests over unit tests. That being said I don't expect us to write an integration test for each property in the API. Most commonly or frequently used properties in that class should suffice. I'd suggest checking with the support engineer or the rest of the XNET team to learn about which properties might be more common or important. For functions or methods, we've been doing a mix of unit and integration tests where applicable. If there's business logic in the python layer or if it's not a simple pass through into the C layer, we've been writing unit tests and I'm hoping we can continue to do that.

I realize "most commonly or frequently" used APIs can be a bit arbitrary and differ based on opinion but I think that's what we've been doing so far. I'm open to other ideas or suggestions on how we should go about deciding when to write unit vs integration or how many integration tests to write, etc. Please let me know if you have ideas or thoughts on this.

jashnani · 2018-10-08T22:00:12Z

nixnet/_enums.py

    PXI = _cconsts.NX_DEV_FORM_PXI
    PCI = _cconsts.NX_DEV_FORM_PCI
    C_SERIES = _cconsts.NX_DEV_FORM_C_SERIES
    PXIE = _cconsts.NX_DEV_FORM_PXIE
    USB = _cconsts.NX_DEV_FORM_USB
+    PCIE = _cconsts.NX_DEV_FORM_PCIE


Very minor. it might help if we sorted the values by bus type or alphabetically (both in code and in docstring) from legibility standpoint.

For example:

C_SERIES
PCI
PCIE
PXI
PXIE
USB

Super minor so not going to hold the review for this.

I agree; sorted is better and there is no need to keep these values in the same order as the C API. Done.

This change adds the new properties listed in ni#255. This change does not address the two new methods for converting byte arrays. Add a new enum value: * DevForm.PCIE Add new error codes: * NX_ERR_LOW_LEVEL_COMMUNICATION_TIMEOUT * NX_ERR_CDAQ_TRANSFER_ABORTED * NX_ERR_DATABASE_IMPORT_VERSION * NX_ERR_EVENT_UNEQUAL_PAYLOAD_LENGTH * NX_ERR_EVENT_UNEQUAL_CHECKSUM_TYPE * NX_ERR_CRIO_BAD_DRIVER_VERSIONS * NX_ERR_CRIO_MISSING_SLOT_SUPPORT * NX_ERR_INTF_ALREADY_IN_USE_BY_NI_XNET * NX_ERR_INTF_ALREADY_IN_USE_BY_NI_XCL * NX_ERR_BYTE_ARRAY_NOT_ALLOWED * NX_ERR_NO_BYTE_ARRAY_MIX * NX_ERR_ONLY_ONE_BYTE_ARRAY Sort existing errors (matches changes made in nixnet.h): * NX_ERR_SESSION_TYPE_FRAME_INCOMPATIBILITY * NX_ERR_DIAGNOSTIC_SCHEDULE_NOT_DEFINED Add two new driver properties, along with unit tests for them: * Interface.lin_checksum_to_in_strm * J1939.include_dest_addr_in_pgn Here is the equivalent C API documentation for the new properties: * http://zone.ni.com/reference/en-XX/help/372841T-01/nixnet/nxpropsession_intflinchecksumtoinstrm/ * http://zone.ni.com/reference/en-XX/help/372841T-01/nixnet/nxpropsession_j1939includedestaddrinpgn/

d-bohls · 2018-10-11T16:02:57Z

After I changed the tests from unit to integration tests, the integration tests started failing on Jenkins because the test machine has an older XNET driver that doesn't support the newer properties. I'll upgrade the XNET driver on the test machine and run the tests again.

d-bohls added the enhancement label Aug 20, 2018

d-bohls requested a review from jashnani August 20, 2018 21:56

epage reviewed Aug 20, 2018

View reviewed changes

epage suggested changes Aug 20, 2018

View reviewed changes

jashnani reviewed Oct 8, 2018

View reviewed changes

d-bohls force-pushed the add_18_0_props branch from 0d2d8c2 to b4f3b36 Compare October 9, 2018 20:56

jashnani approved these changes Oct 10, 2018

View reviewed changes

d-bohls force-pushed the add_18_0_props branch from b4f3b36 to 1fd7732 Compare October 10, 2018 21:57

jashnani merged commit fc369ba into ni:master Oct 11, 2018

d-bohls mentioned this pull request Oct 11, 2018

Expose new API features from NI-XNET 17.0, 17.5 and 18.0 #255

Open

d-bohls deleted the add_18_0_props branch October 11, 2018 18:56

jashnani mentioned this pull request Oct 17, 2018

PCIe form factor definition is missing #253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(API): Add properties available in NI-XNET 18.0. #263

feat(API): Add properties available in NI-XNET 18.0. #263

d-bohls commented Aug 20, 2018

coveralls commented Aug 20, 2018

coveralls commented Aug 20, 2018 •

edited

Loading

epage Aug 20, 2018

d-bohls Aug 20, 2018

epage Aug 20, 2018

d-bohls Aug 20, 2018 •

edited

Loading

jashnani Oct 8, 2018 •

edited

Loading

jashnani left a comment

jashnani Oct 8, 2018 •

edited

Loading

jashnani Oct 8, 2018

d-bohls Oct 9, 2018

d-bohls commented Oct 11, 2018

feat(API): Add properties available in NI-XNET 18.0. #263

feat(API): Add properties available in NI-XNET 18.0. #263

Conversation

d-bohls commented Aug 20, 2018

coveralls commented Aug 20, 2018

coveralls commented Aug 20, 2018 • edited Loading

epage Aug 20, 2018

Choose a reason for hiding this comment

d-bohls Aug 20, 2018

Choose a reason for hiding this comment

epage Aug 20, 2018

Choose a reason for hiding this comment

d-bohls Aug 20, 2018 • edited Loading

Choose a reason for hiding this comment

jashnani Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

jashnani left a comment

Choose a reason for hiding this comment

jashnani Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

jashnani Oct 8, 2018

Choose a reason for hiding this comment

d-bohls Oct 9, 2018

Choose a reason for hiding this comment

d-bohls commented Oct 11, 2018

coveralls commented Aug 20, 2018 •

edited

Loading

d-bohls Aug 20, 2018 •

edited

Loading

jashnani Oct 8, 2018 •

edited

Loading

jashnani Oct 8, 2018 •

edited

Loading