Astemes / astemes-lunit

Unit testing framework for LabVIEW.
MIT License
15 stars 5 forks source link

Remove requirement for starting the name of a test VI with TEST #12

Open antsundq opened 3 weeks ago

antsundq commented 3 weeks ago

In the original implementation of xUnit, test methods are identified by the four letters "test" in the beginning of the name. In many modern implementations for text based languages, this requirement has been dropped and tests are identified by annotations.

One common obstacle when starting to use LUnit is that this requirement is not known by the developer and the tests are not run, because they are not found.

As of v 1.6.15 LUnit also supports the prefix "verify".

Given these facts, it feels like a natural next step to drop the requirement for a prefix all together. Instead any public method belonging to a test case class would be considered as a test method. This should not cause problems as there is no reason to have public methods in a test case class which are not tests.

This would cause the problems that any public method in existing test case classes would be identified as a test. It would not report a result, unless there is an assertion within the VI.

To improve the developer experience we could finally add a result message to any test which does not assert anything to either add an assertion or make it private.

What do you think?

j-medland commented 3 weeks ago

I don't see this imposing any issues with my usage and moving away from relying on vi naming conventions sounds sensible.

antsundq commented 3 weeks ago

Appreciate your feedback John, thanks.

Trying to figure out if there is any use case which would be impaired by such a change.

antsundq commented 3 weeks ago

I realized that there are problems with dynamic dispatch methods. These are used for extending the framework, but also for e.g. the setup.vi and teardown.vi methods.. So I do not want to make these dynamic dispatch methods be identified as tests, but there are valid cases for using dynamic dispatch vi:s as test vi:s (although not recommended for most use cases).

As I see it we can either:

  1. Require dynamic dispatch methods to start with the letters test, while loosening the requirement for static methods
  2. Discard the idea and keep using the prefix

I kind of like the first alternative, but it makes for a possible awkward situation when someone tries to create a dynamic method and it won't run. This is likely an edge case, but it would be frustrating..

j-medland commented 3 weeks ago

Can you just exclude any overrides of the Test Case.lvclass - any child class wouldn't be able to have members called setup.vi or teardown.vi that they intended to use as tests anyway?

antsundq commented 3 weeks ago

That was my first thought. But there might be cases where a test case class needs to define new dynamic vis to be overridden.

This would be rare, but as an example, I'm considering making an add-on for parametrized tests. Then I might want to have an override for defining the parameters. Not sure yet if this is the way to go, but the idea is that you would override one vi to define the sets of parameters you want to test and then every test in that class would be called with these parameters. The problem I'm trying to solve here is duplication of test code.

But then again, treating dynamic and static vis differently really bothers me. Even if using dynamic vis should be very rare, it might end up confusing a beginner.

j-medland commented 3 weeks ago

Ah, yeah. I wasn't thinking about longer inheritance chains.

Maybe just sticking with "Test in the name" convention is fine - at least it is straightforward opt-in to get a VI tested and can be easily communicated.

🤷‍♂️