vipm-io / vi-tester

VI Tester for LabVIEW
BSD 3-Clause "New" or "Revised" License
28 stars 26 forks source link

Feature Request: Allow skipping tests from setUp.vi #38

Closed buckd closed 5 years ago

buckd commented 5 years ago

I have a need to skip a test prior to the actual test case running. My code needs to support LabVIEW/VeriStand versions 2015-2018. My test is written in the oldest LabVIEW version, but some of the hardware I need to test is not supported in the older version, so instead of always failing that test, I would like to skip it if the LabVIEW version is not capable running the test successfully. See this commit for an example.

During setUp.vi, I need to open a connection to VeriStand and then close that connection during tearDown.vi. Ideally, I would be able to detect the version in setUp.vi, and use skip.vi if the version doesn't have the correct support.

skip.vi sets the isSkipped flag in the Test Case class data, but VI Tester doesn't check that flag between setUp.vi and executing the test VI. As a workaround, I tried to just check isSkipped in my test code, but there is no public accessor for that flag, so I have to duplicate the flag in my Test Case subclass and check it prior to executing my test logic and the tearDown code.

I propose checking isSkipped after setUp.vi and ignoring the rest of the code path in that case. Alternatively, a public accessor to isSkipped would at least keep me from having a duplicate flag in every test case where this is a possibility.

jimkring commented 5 years ago

Hi @buckd. Just a quick response to say that this feature request sounds pretty reasonable to me, at first glance. I'll give this a little thought to see if there might be any downside to this. I'm curious if other power users think about this. @kosist @JamesMc86 @omarmussa @francois-normandin

JamesMc86 commented 5 years ago

I could certainly see a use case for this give the variety of environments and versions we run LabVIEW in. No obvious downside to me.

francois-normandin commented 5 years ago

I don't see a problem either for a test to be skipped, but I would not do so by exposing the flag directly to the developer (I see that as breaking encapsulation). Using the skip method should allow a developer to skip a test after setUp.

Python chooses to expose the following methods:

In the later case, a test that is expected to fail would be reported as a failure, but would not count in the overall result... e.g the tests would pass if only expected failures occurred. But that is beyond what @buckd is asking for.

omarmussa commented 5 years ago

One question I have - do you mean TestSuite setup or TestCase setup? I can see the value from TestSuite setup defining a test to skip but I am not sure of the value in the TestCase scenario...

On Feb 6, 2019, at 4:34 AM, Francois Normandin notifications@github.com wrote:

I don't see a problem either for a test to be skipped, but I would not do so by exposing the flag directly to the developer. Using the skip method should allow a developer to skip a test after setUp.

Python chooses to expose the following methods:

skip(reason) skipIf(condition, reason) skipUnless(condition, reason) expectedFailure (reason) In the later case, a test that is expected to fail would be reported as a failure, but would not count in the overall result... e.g the tests would pass if only expected failures occurred. But that is beyond what the OP is asking for.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kosist commented 5 years ago

Flexible conditional skipping of tests - could be quite useful feature! But, I'm not sure about usage of setUp.vi for this. Because at setUp.vi is more logically to set global flag to skip all test methods of that TestCase. Although, quite often there is need to skip just some test methods - and then, solution similar as in Python implementation mentioned by @francois-normandin (https://docs.python.org/3.7/library/unittest.html#skipping-tests-and-expected-failures), could be helpful. Of course, such need also depends on developer's style of unit tests writing and organizing; but if it is more common practice at text-based languages, then it could be followed also for VI Tester...

buckd commented 5 years ago

But, I'm not sure about usage of setUp.vi for this. Because at setUp.vi is more logically to set global flag to skip all test methods of that TestCase. Although, quite often there is need to skip just some test methods - and then, solution similar as in Python implementation mentioned by @francois-normandin (https://docs.python.org/3.7/library/unittest.html#skipping-tests-and-expected-failures), could be helpful.

For the specific test I have, I am skipping all test methods in a test case because I have the same hardware setup for all the methods, so setUp.vi works well for me in this case. I wouldn't be opposed to an expected failure option if that's what is decided. Basically, I don't want my CI to report tests failed and prevent pull requests/system deployment because I'm testing a supported version of software, but that version just doesn't support the feature in this test case. And managing different suites per LV/VeriStand version becomes unwieldy quickly.

jimkring commented 5 years ago

Yes, the TestCase.lvclass has a skip.vi [skip(reason)] method that can be called.

image

I've been playing around with a change (inside TestCase.lvclass:run.vi) that I think will work.

image

Here's what it looked like before my mods.

image

I've tested the change and it seems to work, OK.

image

Anyone see anything I'm missing?

jimkring commented 5 years ago

Here is a pre-release build of the package you can try installing and testing: https://github.com/JKISoftware/JKI-VI-Tester/releases/tag/3.0.1

I'm making these changes in the allow-skipping-test-inside-setup branch.

buckd commented 5 years ago

Had time to get back to this today. This does exactly what I had in mind.

jimkring commented 5 years ago

Glad it works for you @buckd. You can download this with VIPM (release 3.0.2)