Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Spread execution of unit tests over time #95

Closed
Roald87 opened this issue Mar 19, 2020 · 34 comments
Closed

Spread execution of unit tests over time #95

Roald87 opened this issue Mar 19, 2020 · 34 comments
Labels
enhancement New feature or request

Comments

@Roald87
Copy link
Contributor

Roald87 commented Mar 19, 2020

When a lot of unit tests or calculation intensive tests have to be executed, the task cycle time can get exceeded. If the cycle times exceeds, unit tests can fail which make use of timers.

Maybe it is a good idea to somehow enable the spreading of the execution of unit tests?

@sagatowski
Copy link
Member

sagatowski commented Mar 22, 2020

Good idea!

How would the spreading work? What would the criteria be for setting these times?

Remember that you can set a separate cycle-time for the unit tests. There might even be a way to ignore cycle-overruns (not create them at all) for specific tasks?

This is also related to #54.

@Roald87
Copy link
Contributor Author

Roald87 commented Mar 23, 2020

I made a first implementation which works as follows. I added a function DELAYED_TEST('TestName', DelayCycles:=5) which you can use to register the test and set a number of delay cycles. The function returns a boolean, which will become true when the number of cycles you've set are passed.

Example usage:

IF DELAYED_TEST('TestName', DelayCycles:=5) THEN
    // Unit test code
END_IF;

I think running it on a longer cycle time will only go so far. You bump into the limit again when you add more tests. Also you need to reserve another core for this longer cycle time I think? I'm not sure if there is a way to ignore cycle overruns.

As I mentioned the reason I opened this issue is that I was having problems with the timers. My first solution was to just delay all unit tests until my normal code had initialized, since the normal code of my project is quite computationally intensive in the first few cycles. However, this didn't work because apparently TcUnit.Run() needs to be called in the first cycle, or else the tests do not get registered. That's why I made the implementation I mentioned in the beginning.

Relating this to #54, I could also work on this. Implementation could be done similar to the delayed test. For example add another method TEST_WITH_TIMEOUT('TestName', TimeOut:=T#1s). This method will be true as long as the timer has not expired. Once the timer expires the method will return false.

IF TEST_WITH_TIMEOUT('TestName', TimeOut:=T#1s) THEN
    // Unit test code
END_IF;

Finally maybe we could make TEST also return a boolean to signify is a test is already done? Then it would be a bit easier to prevent tests from running multiple times and the usage would be the same as TEST_WITH_TIMEOUT and DELAYED_TEST.

What do you think about it?

@sagatowski
Copy link
Member

sagatowski commented Dec 6, 2020

Hi @Roald87! Thanks for all your great input/suggestions and I'm really sorry for late response. We've had like three minor build of TwinCAT since you wrote this issue initially. Shame on me 🙈
I highly appreciate all the time and effort you have put into this. There are more and more people writing and asking for this type of functionality now, so I think the time has come to actually do something about it. Your suggestion is one way to do it, although the obvious drawback is that it can get a bit messy once we add more and more tests. I see three ways we could handle this:

  1. Your suggestion. For instance, if we add a delayed_test, then the developer can decide which tests should run in which order, although not in a sequence after each other but only relative to time. The drawback is of course that the tests will be dependent on the CPU, and the tests will have an (indirect) dependency to each other (which is one of the no-no's of writing tests, see this, and more particularly Ignore assert-prints if test is a duplicate #8). When writing an unit tests, I don't want to consider (in time) for how long the previous tests have been running, and I don't want to consider which CPU they have been running on as well (unless this is part of the test!). To be fair, Ignore assert-prints if test is a duplicate #8 is not 100% applicable for TwinCAT unit tests, as the test in itself might be that it needs to finish in a certain time on a certain platform, and that's a good reason in itself to have tests run one after another (but only once the previous once are finished). I like the simplicity of this solution, and it could even be included as an option as it doesn't break any existing functionality.
  2. Add a way to run test suites in a specific order. Right now, if you declare a test suite (by instantiating a test-suite-FB), it will just go into a pool of test suites, and they are run in the order that they were declared. If the user could (somehow) declare which order a certain test-suite should be running in, a test-suite wouldn't be running until all tests in the previous test-suite were finished. The drawbacks of this is that I (right now) don't see how to implement this without breaking the current TcUnit API (maybe you have a suggestion?), and secondly that the resolution would only be on test-suites and not on test-cases. One could argue that in that case you could just make a separate test-suite for these special test cases though. There are a few fault cases that would have to be considered as well (what if a test-suite is not declared to be in a specific order, while the other are? What if two test-suites are set in the same order)? etc.
  3. A variant of (1), or maybe an addition. Maybe have a TEST_ORDERED('TestName, #number) to declare in which order a test should run within a test-suite (so not on an overall level for all test-suites, but only within a test-suite. So we could have a structure like:
  • TEST_SUITE#1
    --- TEST_ORDERED('Hello', 1)
    --- TEST_ORDERED('HelloAgain', 2)
    --- TEST_ORDERED('HelloAgainAndAgain, 3)
  • TEST_SUITE#2
    --- TEST_ORDERED('Blabla', 1)
    --- TEST_ORDERED('BlablaBla', 2)
    --- TEST_ORDERED('BlablaBlaBla', 3)
    --- TEST_ORDERED('BlablaBlaBlaBla', 4)
    And if you mix normal tests (TEST('name')) with these, they are run as usual (that is, together with the ordered tests). It would work the same way as the TEST_DELAYED, so basically returning a boolean to know whether we should execute the code in the test, such as:
IF TEST_ORDERED('Hello', 1) THEN
   // Do stuff
   TEST_FINISHED();
END_IF

I've been looking at how other unit-testing frameworks do this, and although it's not fair to do a comparison of "IT"-unit testing frameworks such as JUnit, there still seem to be some need for this in here as well. JUnit recently added support:
https://github.com/junit-team/junit4/wiki/test-execution-order

Obviously I'm afraid to break any existing TcUnit users functionality. If anyone downloads the next TcUnit-release and re-runs their existing tests, they should produce the exact same result, so this needs to be taken into consideration.

@Roald87, let me know your thoughts. As I've had numerous people e-mail me over this now, it seems to be important :-)
Is there a fourth and maybe even a fifth alternative? Please brainstorm!

@Roald87
Copy link
Contributor Author

Roald87 commented Dec 10, 2020

Hi @sagatowski!

No problem. I know it is difficult to work on a project when you have a job and a life next to it :p. Glad to help and support the project when I can!

Can you maybe clarify something first? Are people requesting ordered tests or spreading tests? My goal was to spread unit tests over time, to prevent failures because of cycle time exceeds. You are focusing on test order. Of course with my suggested implementation there will be a certain order in the tests, but that is more a side effect than the goal.

As you also rightfully note that there should not be any order in the tests. Won't implementing ordered test only encourage "wrong" unit test writing?

@sagatowski
Copy link
Member

Hi @Roald87!
Thanks for fast response.

The requested feature is to purely be able to spread the execution of the tests. I've had a few requests now where users want to use TcUnit on cheap PLCs.

So you are entirely correct, tests should not be dependable on each other or the running order, but in this case it's just a side effect of achieving the goal of spreading the execution (test of course still should be written in such a way that the outcome of one test should not affect the outcome of another test).

@Roald87
Copy link
Contributor Author

Roald87 commented Dec 11, 2020

The drawback is of course that the tests will be dependent on the CPU, and the tests will have an (indirect) dependency to each other

Not sure if I understand this. The DELAYED_TEST will only say when a certain test is allowed to run for the first time. It doesn't care if a previous test has already finished or not. Or am I missing something?

The drawbacks of this is that I (right now) don't see how to implement this without breaking the current TcUnit API (maybe you have a suggestion?)

Make a new FB: FB_SequentailTestSuite EXTENDS FB_TestSuite? It doesn't break the current API, but adds to it. All sequential FBs are then run in sequence once all normal test suites have executed. This could be a more convenient solution, because with DELAYED_TEST you need to add so much extra code.

Until now I have been using my DELAYED_TEST('TestName', CyclesToWait:=23), but I'm not quite sold on the concept. It is very inconvenient to tune how many tests should start when. If you want to shift all tests by e.g. 1 cycle, or have less tests per cycle you need to alter every individual method. Or as I did recently, write bash scripts which do that for me x). The latter is not an option obviously :p.

I just thought of an alternative. What about adding a method SPREAD_TEST('TestName')? Usage would be similar as DELAYED_TEST

IF SPREAD_TEST('TestName') THEN
   // Do stuff
   TEST_FINISHED();
END_IF

And then SPREAD_TEST would either read from a global variable, input from a test suite, or .. ? where the user can set the maximum number of tests which are allowed to start in a cycle. Then the method itself would assign a CycleToStart to that test, based on the maximum number of tests which are allowed to run and how many tests are already set to run in cycle x.

@sagatowski
Copy link
Member

The drawback is of course that the tests will be dependent on the CPU, and the tests will have an (indirect) dependency to each other

Not sure if I understand this. The DELAYED_TEST will only say when a certain test is allowed to run for the first time. It doesn't care if a previous test has already finished or not. Or am I missing something?

But if you were to do a delayed test, wouldn't you want it to be delayed long enough so that all previous tests would be finished? Wasn't the idea to make sure that one test could run, finished, and then the next one etc in a sequence? (so that we wouldn't get exceed overruns?) Or do we possibly talk about apples and pears here? :-)

The drawbacks of this is that I (right now) don't see how to implement this without breaking the current TcUnit API (maybe you have a suggestion?)

Make a new FB: FB_SequentailTestSuite EXTENDS FB_TestSuite? It doesn't break the current API, but adds to it. All sequential FBs are then run in sequence once all normal test suites have executed. This could be a more convenient solution, because with DELAYED_TEST you need to add so much extra code.

Wouldn't this still require to have a special TEST()-method? The thing here is that we don't want any code to run inside of a test unless it's time for it to run. Wouldn't that still require us to somehow define sequence order of tests?

Until now I have been using my DELAYED_TEST('TestName', CyclesToWait:=23), but I'm not quite sold on the concept. It is very inconvenient to tune how many tests should start when. If you want to shift all tests by e.g. 1 cycle, or have less tests per cycle you need to alter every individual method. Or as I did recently, write bash scripts which do that for me x). The latter is not an option obviously :p.

Yes, exactly this is what I want to avoid, that you should not have to care about how much time any previous (or following) tests took.

I just thought of an alternative. What about adding a method SPREAD_TEST('TestName')? Usage would be similar as DELAYED_TEST

IF SPREAD_TEST('TestName') THEN
   // Do stuff
   TEST_FINISHED();
END_IF

And then SPREAD_TEST would either read from a global variable, input from a test suite, or .. ? where the user can set the maximum number of tests which are allowed to start in a cycle. Then the method itself would assign a CycleToStart to that test, based on the maximum number of tests which are allowed to run and how many tests are already set to run in cycle x.

But wouldn't that mean that the tests would still run parallell with each other (but only those one defined in spread_test()?)

The use case that people have been asking about is to solve the problem that they get exceed overruns, because all tests are run in parallell, and they want to be able to run them in sequence (one after the other, so that it's possible to do tests on a target CPU and now have timers that are timing out because of overruns). I just want to make sure we have the same goal :-)

@Roald87
Copy link
Contributor Author

Roald87 commented Dec 20, 2020

Wasn't the idea to make sure that one test could run, finished, and then the next one etc in a sequence? (so that we wouldn't get exceed overruns?) Or do we possibly talk about apples and pears here? :-)

Maybe I was thinking of a simpler approach. Of course running al your tests in sequence would be the safest bet that there are no exceed overruns. I think that my suggestion with SPREAD_TEST would probably already solve most overruns and it is easier to implement. At least I think it is easier :P.

Wouldn't this still require to have a special TEST()-method? The thing here is that we don't want any code to run inside of a test unless it's time for it to run. Wouldn't that still require us to somehow define sequence order of tests?

Yeah probably. I think that this approach would only enable test suites to run in sequence, not the tests themselves.

@Beidendorfer
Copy link
Contributor

Hi guys,

I would like to join the discussion.
I come to this issue from another problem. With more and more unit test I need to adjust the AdsLogMessageFifoRingBufferSize. Since the results are all coming in 2-4 Cycles. #88
I think it would be better, if the tests can be run one after the other = more Cycles

My idea: Only the test suits are executed one after the other

I would write 2 new FUN.
image
1.) RUN_SEQUENCE

This function calls a new method in FB_TcUnitRunner
RunTestSuiteTestsSequence

Here the For loop is replaced and the counter is incremented when the TestSuit is finished.
It would call one TestSuit after the other

2.) RUN_SEQUENCE_DELAYED

This function calls a new method in FB_TcUnitRunner
RunTestSuiteTestsSequenceDelay

The same implementation as 1.) only that before increasing the counter of the testsuits a time x is waited. So the time between two TestSuits can be set. This is to get possibly more time for the evaluation of the ADSBuffer.

image

If now the TestSuits run one after the other, the evaluation must be adapted to this process.
Therefore a new FB_TestResultsSequence is needed, where all For loop get replaced by a trigger TestSuitFinihsed

image

This new FB will only be called from the 2 new Methodes RunTestSuiteTestsSequence and RunTestSuiteTestsSequenceDelay

With such an extension it should be possible to have the program downward compatible

When switching between parallel and sequence processing only TcUnit.RUN_SEQUENCE instead of TcUnit.RUN is required.

The testsuits run according to the instantiation order.
For debugging I would create a message log with a timer e.g. 1 minute, which outputs the last current TestSuite. If there should be problems with a TestSuit you can find out which one it was.

Would this be a possibility that could be considered?
Have I overseen anything?

@sagatowski
Copy link
Member

@Beidendorfer I think the suggestion with RUN_SEQUENCE is really good! It solves the most urgent problem with all tests running at the same time, although I think that we probably might need a more fine-grained way to run the individual tests in a sequence as well. Anyhow, with running the test-suites sequantially this gives us the possibility to separate them if we need to run tests in a sequence.

I'll look into this as I personally need this type of functionality for a project I use myself.

@Aliazzzz
Copy link
Contributor

Aliazzzz commented Dec 29, 2020

Hello,

I thought maybe the chain of responsibility pattern could help out in this matter in order to streamline the tests atomically one by one.
https://sourcemaking.com/design_patterns/chain_of_responsibility

image

@sagatowski sagatowski added this to the Release 1.2.0.0 milestone Dec 30, 2020
@sagatowski
Copy link
Member

@Beidendorfer Gahhh this was a much harder problem to solve than I initially thought. I'll hopefully have something commited today or latest tomorrow for test. Initially I will include the RUN_IN_SEQUENCE(), but for release 1.2 of TcUnit I would like to have something where it's possible to run individiual tests in a sequence/ordered as well.

@Beidendorfer
Copy link
Contributor

@sagatowski

Gahhh this was a much harder problem to solve than I initially thought.
That's what I thought, It needed to adjust the ads logging to the sequence.

When the code is pushed I'll have a look at it, I'm curious which implementation it has become.

For the ordering I could imagine FB_TestSuite_Ordered which extends FB_TestSuite.
Added in the FB_INIT an iOrderID which specifies the order.

For the sequence I could imagine, that first the FB_TestSuite_ordered are processed in the order and then the normal FB_TestSuits

The programmers are responsible for the correct order iOrderID's.
If there are nevertheless once this same ID 2 times, the second Id is executed after the first.
e.g. iOrderID 1,2,3,3,4,5
Does this make sense?

I can't tell how huge the work is to adjust the reporting to the new order.

Another question: Is it possible to run Run() and RUN_IN_SEQUENCE() in parallel?

@sagatowski
Copy link
Member

You don't need any special FB_TestSuite_Ordered. The only difference for the developer is to call RUN_IN_SEQUENCE() instead of RUN(). The order of execution is the order in which you have declared the test suites.

For running the tests, and specifying the order, I thought the best thing is to do it the same way as test suites, that is calling the tests in the order you want to run them. In this way the developer doesn't need to:

  • Remember which id's are taken and not, and making sure to specify them in the right order for all test-functions
  • We don't need to take into account that the same ID has been used twice

RUN and RUN_IN_SEQUENCE() can not be run in parallel.

@Beidendorfer
Copy link
Contributor

For running the tests, and specifying the order, I thought the best thing is to do it the same way as test suites, that is calling the tests in the order you want to run them. In this way the developer doesn't need to:

If the order of the test in RUN_IN_SEQUENCE() then looks like the following.
first FB_TestSuite instantiated, first Test method and then first Test?

Or is there a TEST_IN_SEQUENCE() so that the test order can also be used in Run() and RUN_In_Sequence()?

RUN and RUN_IN_SEQUENCE() can not be run in parallel.

Maybe we should lock that internally, that only one can run and otherwise abort and give out an error message.

I am very glad that this extension Run_IN_Sequence() is coming.

sagatowski added a commit that referenced this issue Dec 30, 2020
This executes the test suites one after the other (as opposed to RUN() which executes all test suites in parallel).
For more details, see #95.

Also changed method access modifiers for all methods in TcUnit so that none is left with the default (PUBLIC).
Incremented version of TcUnit to 1.2.0.0.
@sagatowski
Copy link
Member

Added the RUN_IN_SEQUENCE(). Please see commit d152c95 for details.

@sagatowski
Copy link
Member

sagatowski commented Dec 30, 2020

For running the tests, and specifying the order, I thought the best thing is to do it the same way as test suites, that is calling the tests in the order you want to run them. In this way the developer doesn't need to:

If the order of the test in RUN_IN_SEQUENCE() then looks like the following.
first FB_TestSuite instantiated, first Test method and then first Test?

If we have two test suites:

fbTestSuite1 : FB_TestSuite1;
fbTestSuite2 : FB_TestSuite2;

Then all tests in fbTestSuite1 are first executed (at the same time), and then all tests in fbTestSuite2 are executed (but only after all tests in fbTestSuite1 are finished).

Or is there a TEST_IN_SEQUENCE() so that the test order can also be used in Run() and RUN_In_Sequence()?

Yes I think we should also add the TEST_ORDERED to this.
Then it's possible to either setup the RUN_IN_SEQUENCE() (for test suites ordering) and/or TEST_ORDERED() if you want to define the order of the individual tests or even both!
This means you could either run:

  • RUN() with all tests as TEST() or
  • RUN() with all tests as TEST_ORDERED() or
  • RUN() with mixed tests of TEST() and TEST_ORDERED()
  • RUN_IN_SEQUENCE() with all tests as TEST() or
  • RUN_IN_SEQUENCE() with all tests as TEST_ORDERED()
  • RUN_IN_SEQUENCE() with mixed tests of TEST() and TEST_ORDERED()

This should give maximum flexibility of how to run the tests, without being too confusing for the user.

RUN and RUN_IN_SEQUENCE() can not be run in parallel.

Maybe we should lock that internally, that only one can run and otherwise abort and give out an error message.

good idea!

I am very glad that this extension Run_IN_Sequence() is coming.

me too 😃

@sagatowski
Copy link
Member

For the upcoming release of TcUnit 1.2.0.0 I've written some instructions of this new functionality in the FAQ of TcUnit. Please review and comment if you think something is unclear/missing:
https://tcunit.org/frequently-asked-questions/?Display_FAQ=970

@Beidendorfer
Copy link
Contributor

Hi I have made the first tests. First of all, thanks @sagatowski for the implementation.

I ran some worst-case tests.
10 test suites each having 216 test and AdsLogMessageFifoRingBufferSize = 2000(default)
MaxNumberOfTestsForEachTestSuite = 220 ( customized)

After approx. 2000 messages TC Unit stops without making an evaluation, no error message, no indication whether TCUnit is still working.

When I increase the ADS memory
AdsLogMessageFifoRingBufferSize = 8000 instead of 2000
TcUnit works with Run() and RUN_In_Sequence()

For performance reasons I want to keep the memory small
AdsLogMessageFifoRingBufferSize = 2000

To achieve this, I started the 10 TestSuits one after the other with a delay.(10s)
This gave the asynchronous ADS router more time to process the many messages.
The test suits now run one after the other but still very fast for the ADS Rounter. Instead of 1 cycle, now 10 cycles for my worst-case test.

Now I can not only start the individual tests 1 after another but also distribute them over a period of time (cycles).
I think this also helps with small hardware performance

My implementation:
Added a Time in RUN_IN_SEQUENCE
image

pass this to RunTestSuiteTestsInSequence
image

changed Code

**TofWaitingTime(PT:=tDelayTime);**
(* Run TcUnit test suites *)
IF NOT AllTestSuitesFinished THEN
    IF GVL_TcUnit.NumberOfInitializedTestSuites = 0 THEN
        AllTestSuitesFinished := TRUE;
    ELSIF GVL_TcUnit.NumberOfInitializedTestSuites > 0 THEN
		**IF TofWaitingTime.Q THEN
			TofWaitingTime.IN := FALSE;
		END_IF**
        IF GVL_TcUnit.TestSuiteAddresses[CurrentlyRunningTestSuite]^.AreAllTestsFinished() THEN
            IF CurrentlyRunningTestSuite <> GVL_TcUnit.NumberOfInitializedTestSuites THEN
                NumberOfTestSuitesFinished := NumberOfTestSuitesFinished + 1;
                CurrentlyRunningTestSuite := CurrentlyRunningTestSuite + 1;
		**TofWaitingTime.IN := TRUE;**
            END_IF
        **ELSIF NOT TofWaitingTime.Q THEN**
            GVL_TcUnit.CurrentTestSuiteBeingCalled := GVL_TcUnit.TestSuiteAddresses[CurrentlyRunningTestSuite];
            GVL_TcUnit.CurrentTestSuiteBeingCalled^.SetHasStartedRunning();
            GVL_TcUnit.CurrentTestSuiteBeingCalled^();
        END_IF

now it waits a Time between one Test Suite and another.
If I do not want a time delay I enter T#0s. then one test suite is started right after each other

What is your opinion? Is it just a special case or interesting for everyone? I think it is a combination with @Roald87 idea.

@sagatowski
Copy link
Member

sagatowski commented Dec 31, 2020

@Beidendorfer Although I find it a very special use case with over 10 test suites with over 200 test cases in each, I also like the idea that this makes it possible to run tests on less capable machines (such as the CX8100-series or the new CX7000-series) that most likely will have even less capable ADS routers. I wish the problem with too many ADS-messages didn't exist, but with this reality and the fact that there probably are people using tiny/low performant PLCs just to run their tests (especially in a CI/CD toolchain with TcUnit-Runner), I think yours and @Roald87 suggestion is great.

Could you do a PR with your code?

Thanks for this suggestion!

Edit: Thanks for putting your time and effort into doing this investigation and test. It's highly appreciated.

@sagatowski
Copy link
Member

@Roald87 would this solution (RUN_IN_SEQUENCE() + TEST_ORDERED()) be applicable/useful for the tests in where you are using the DELAYED_TEST()?

Time to stop coding and prepare for some new years celebrations. Happy new 2021 to you @Roald87 @Beidendorfer @Aliazzzz !

@Roald87
Copy link
Contributor Author

Roald87 commented Jan 1, 2021

I think the RUN_IN_SEQUENCE() would already be enough, since that would spread the tests over multiple cycles.

sagatowski added a commit that referenced this issue Jan 2, 2021
…sue #95.

Merged changes from @Beidendorfer that allows RUN_IN_SEQUENCE() to have a time that defines how long the time will be between test suites and that solves issue #128.
Reverted changes where <LineId> was introduced and TwinCAT project files were updated to 4024 (whilst it should use 4022).
@sagatowski
Copy link
Member

I've just made a commit that includes the TEST_ORDERED.
Verified that all tests still pass with TcUnit-Verifier. Please test TEST_ORDERED() if you want.

See 26f40b4 for all details.

@sagatowski
Copy link
Member

sagatowski commented Jan 2, 2021

I've also created three example TwinCAT projects (+ describing documentation) for some examples of how to run test suites and tests in sequence.
https://github.com/tcunit/ExampleProjects/tree/master/RunTestsInSequenceExampleProjects

I think we are getting close to closing this issue.

@RodneyRichardson
Copy link

I like the idea of TEST_ORDERED to allow the developer to limit the number of concurrent tests occurring in a TestSuite, and RUN_IN_SEQUENCE to limit the number of concurrent TestSuites to 1.

Can I suggest an enhancement? Have a MAX_CONCURRENT_TEST_SUITES parameter (input to RUN_IN_SEQUENCE or even RUN) to allow multiple suites to run concurrently. This would allow a developer to speed up the testing run, but still allow them to limit resource usage.

@Roald87
Copy link
Contributor Author

Roald87 commented Feb 22, 2021

I tried RUN_IN_SEQUENCE today, but it didn't not quite work for me yet. The main point is that the tests take quite long if one or more test suites have a test which has to wait for a timer to elapse.

I think @RodneyRichardson has a good suggestion by allowing multiple concurrent test suites to run. Maybe an even better (or additional) solution would be to have a MAX_CONCURRENT_TESTS, because different test suites can have very different number of tests. Although having this option would require two things:

  1. Wrap all TEST in an IF statement
  2. Have TEST return TRUE when it is ready/allowed to run.

Some other suggestions:

  • Add an initial delay for the test suites to start running. I now just solved it by adding a dummy test suite with a single test which finishes after a counter reached a certain number.
  • Make TEST return a boolean, also in case MAX_CONCURRENT_TESTS is not implemented. TEST would return true if a test is not done/failed. This way only tests are run (if wrapped in an if block) which have not finished yet.

@sagatowski
Copy link
Member

sagatowski commented Feb 22, 2021

@RodneyRichardson Thanks for the suggestion. This is possible to do today, but only with TESTS, and not TEST-SUITES. The question that arises is though how we prioritize the test-suites? If you only want two test-suites to be executed at the same time, how do we know which ones should go first? Declaration decides? More parameters?

@Roald87, do you mean there is a bug or is it just a general notation that it doesn't work for your usecase?

If a test takes too long time, why don't you run it as a TEST() and not TEST_ORDERED()? Then it will run in parallell with everything else that is TEST_ORDERED(). Or have I missed something?
The functionality of it is described here:
https://tcunit.org/frequently-asked-questions/?Display_FAQ=970
I've recently added this with descriptive pictures. This means you can run TEST() with TEST_ORDERED() combined. So you don't have to chose one or the other, but you can combine the two.
Edit: I've also created some examples that demonstrate this: https://github.com/tcunit/ExampleProjects/tree/master/RunTestsInSequenceExampleProjects
Especially MixedSequentialAndParallelTests might be interesting.

All these suggestions make me realize that I will most likely remove the TimeBetweenTestSuitesExecution input-parameter from RUN_IN_SEQUENCE() and add it as a GVL_Param instead, as this will allow for maximum flexibility for any future additions without having to break the backward-compability. The drawback of this is though that it's less obvious to change a parameter in some weird GVL_Param rather than as an input-field. So many choices and opinions, so little time... :-)

Regarding the point with initial delay for the test-suites, I think this is what TimeBetweenTestSuitesExecution will solve, no?

Again, 1.2 is not yet released so things can still change.

@Roald87
Copy link
Contributor Author

Roald87 commented Feb 23, 2021

It didn't work for my use case.

Currently I have some custom methods, which delay tests by a certain number of cycles. I got this to work with some trail and error. I wanted to see if I also could run the tests by converting to v1.2 and running all tests in sequence using RUN_IN_SEQUENCE and using TEST for individual tests. This worked when I enabled about half of my tests, but the tests took much longer to complete and the CPU was often at 99%. When I enabled all tests I got an exception, probably because there were too many cycle overruns. I didn't want to put too much time into investigating this, since I already had a somewhat working system.

I could probably optimize the tests by using a mix of TEST() and TEST_ORDERED, however that would be a lot of work since there are 200+ tests. I guess the new methods would work if I started from scratch. However, in my case converting existing unit tests was a bit too much work.

Regarding the point with initial delay for the test-suites, I think this is what TimeBetweenTestSuitesExecution will solve, no?

I meant an initial delay for all the unit tests to start.

@sagatowski
Copy link
Member

So if you changed your TEST()'s into TEST_ORDERED's (or at least some of them), it would solve the problem?

Why is an initial delay to start all test suites necessary?

@Roald87
Copy link
Contributor Author

Roald87 commented Feb 23, 2021

So if you changed your TEST()'s into TEST_ORDERED's (or at least some of them), it would solve the problem?

I think so yes.

Why is an initial delay to start all test suites necessary?

There is a lot of cpu intensive initialization going on in this project. So it is better to wait with the unit tests.

@sagatowski
Copy link
Member

sagatowski commented Feb 24, 2021

There is a lot of cpu intensive initialization going on in this project. So it is better to wait with the unit tests.

I see, never thought of this use case. I guess one option would be to add a parameter where you specify how long TcUnit should wait before it executes the tests, would that solve that particular problem?

Edit: All of this makes me really curious of how you and other are using TcUnit. Should have a "TcUnit users group"-day ;-)

@Roald87
Copy link
Contributor Author

Roald87 commented Feb 24, 2021

I see, never thought of this use case. I guess one option would be to add a parameter where you specify how long TcUnit should wait before it executes the tests, would that solve that particular problem?

That would solve it indeed. I also once tried to put a timer around TcUnit.Run(), but then the tests were not initialized and they did not run. I didn't look too much into why it didn't work.

Edit: All of this makes me really curious of how you and other are using TcUnit. Should have a "TcUnit users group"-day ;-)

That would be an online meeting then, but big online meetings are not really a success usually. Even with people you know. But it would be nice to exchange ideas.

@sagatowski
Copy link
Member

I consider this issue to be solved.

@sagatowski
Copy link
Member

These changes have been made that are related to this issue.

  1. Moved "TimeBetweenTestSuitesExecution" from ORDERED_SEQUENCE() to parameter - Time delay between a test suite is finished and the execution of the next test suite starts if using RUN_IN_SEQUENCE(), see this commit.

  2. Updated examples to reflect this, see this commit.

  3. Added an FAQ entry to the TcUnit website: https://tcunit.org/frequently-asked-questions/?Display_FAQ=1008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants