#5 automated tests

[timyhac]

Hey there @jkoplo - was hoping to get your opinion on how these tests are structured.

In order to set up the test harness I need to remove the direct dependency of Tag on the native plctag methods, so I created INativeTag and added an indirection layer to the native library.

Main thing that I'm not sure about is that I needed to remove the sealed keyword from Tag - I'm not totally clear on why that was there. To be able to mock the dependency on the native library, I added a private constructor to allow dependency injection of an INativeTag implementation (i.e. a mock) but keeping that constructor invisible from the public API for Tag.

[timyhac]

Actually, I've updated this slightly so now we have a few layers of indirection.

The end result:

• The real logic has been moved to an internal class NativeTagWrapper.
• The public Tag class is sealed.
• NativeTagWrapper is exposed to the test project and the NativeTag interface can be mocked.

One thing I noted was that Tag<M,T> isn't sealed, but Tag is - I'm not super clear on why that is.

[jkoplo]

I'll try to take a look this weekend. Real life has been getting in the way, but I'd like to get back into this project some more...

[timyhac]

@jkoplo - I'm thinking it may be useful if I merge this work but do not publish anything to nuget until we've both agreed on testing approach. Are you ok with that?

Update: not so much the testing approach, I don't think we have many options besides what has been done here, but more in terms of what the public API should be after integrating this.

[jkoplo]

Okay, so it looks like everything that was in Tag became NativeTagWrapper' (all the additional code that was provided beyond the native libplctag). And the new Tagis just a thin re-implementation ofNativeTagWrapper` that injects 'NativeTag' by default.

And then a NativeTag was created that's one-for-one with libplctag and implements the new INativeTag.

This allows us to test NativeTagWrapper but still keep Tag simple for library users. I think I follow...

[jkoplo]

Thinking through this more, here's our layers from highest-level to lowest-level:

1. TagOfT (Really Tag<M, T>) - Object that has a nice clean surface and allows for native object types.
2. Tag - Still user-facing, but a lower level way to use tags.
3. NativeTagWrapper - Internal. Responsible for enforcing order of operations with some .NET friendly handling.
4. NativeTag - Internal. Exists just to make libplctag.NativeImport.plctag injectable against INativeTag.
5. libplctag.NativeImport.plctag - Wraps the C DLL into .NET calls.

Could you simplify this by making INativeTag part of libplctag.NativeImport and removing NativeTag? Or does that play havoc with dependencies and access levels?

On the unit-test side of things, it looks pretty good. I'll be honest that I only have an academic understanding of unit testing. Other than using and modifying other people's tests on a few open-source projects, I've never written them from scratch for my own code.

[timyhac]

"Or does that play havoc with dependencies and access levels?" - yes, we would need to expose INativeTag and NativeTag in NativeImport for that to work - it would mean that the test project would (directly) depend on the NativeImport library.

Static methods can't be used to implement an interface, so we would still need the NativeTag type, it would just be moved to NativeImport.

[kyle-github]

Is there something I can do in the core lib to help this? Would leveraging the callback API make this easier?

[timyhac]

I think the original purpose (September 2019) of this ticket (#5) has changed somewhat.

Originally this was going to test what has essentially become the NativeImport library. I'm not sure how to test this beyond manually testing to be honest. There was an idea to have a stub dll so that we could just check that the appropriate native method was called, but I'm not sure what the value of that is.

The purpose of this is now to test the stuff that the primary library adds on top of the native library (disposal of system resources, separation of async/sync, mapping of PLC types into .NET types) - so in fact I think it is better that the core library is not involved at all, otherwise the boundaries of the system-under-test blur.

[jkoplo]

Yeah, I see now why that would cause havoc.

From my perspective this looks solid, but we should probably document how the pieces fit together somewhere.

I don't think there's a ton of value in testing NativeImport package. We have to trust that the C library is good (via it's own testing). All we'd actually be testing is the .NET calls into C, and that's not real useful. The only other thing we could possibly test would be the extraction and override scenarios.

I think we merge this.

[timyhac]

Yep I agree with that sentiment @jkoplo