Skip to content

Conversation

@philj404
Copy link
Contributor

@philj404 philj404 commented Nov 4, 2020

Adds Makefiles and a GitHub action which will run arduino-timer unit tests in a Linux build environment.

A passing result helps give confidence that a code change/new feature did not cause a regression.

Information from a failing result can give hints on what to investigate.

See issue #20 for more info.

--
This is a follow-on to #19 . It adds continuous integration capability to regression testing.

Since it's a follow-on, you could just merge this PR and get the other one "for free". But since it's a different feature I was working on, I wanted to keep the two perspectives separate.

Comment on lines 7 to 8
*.out
*.o
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UnixHostDuino generates output files/executables which we shouldn't put into the repo.

@philj404 philj404 marked this pull request as ready for review November 4, 2020 01:37
@philj404 philj404 changed the title Feature/ci unit tests Feature/ci unit tests on push Nov 4, 2020
@contrem
Copy link
Owner

contrem commented Nov 5, 2020

Please fix the warnings:

timerTest.ino: In member function ‘virtual void test_timer_at::once()’:
timerTest.ino:158:8: warning: unused variable ‘atTask’ [-Wunused-variable]
  158 |   auto atTask = timer.at(atTime, DummyTask::runATask, &waste_3ms);
      |        ^~~~~~
timerTest.ino: In member function ‘virtual void test_timer_in::once()’:
timerTest.ino:199:8: warning: unused variable ‘atTask’ [-Wunused-variable]
  199 |   auto atTask = timer.in(delayTime, DummyTask::runATask, &waste_3ms);
      |        ^~~~~~
timerTest.ino: In member function ‘virtual void test_timer_every::once()’:
timerTest.ino:241:8: warning: unused variable ‘everyTask1’ [-Wunused-variable]
  241 |   auto everyTask1 = timer.every(50, DummyTask::runATask, &waste_3ms);
      |        ^~~~~~~~~~
timerTest.ino:242:8: warning: unused variable ‘everyTask2’ [-Wunused-variable]
  242 |   auto everyTask2 = timer.every(200, DummyTask::runATask, &waste_100ms_once);
      |        ^~~~~~~~~~
timerTest.ino: In member function ‘virtual void test_timer_delayToNextEvent::once()’:
timerTest.ino:295:22: warning: comparison of integer expressions of different signedness: ‘long unsigned int’ and ‘int’ [-Wsign-compare]
  295 |   while (simMillis() < start + 1000) {
      |          ~~~~~~~~~~~~^~~~~~~~~~~~~~

This last one is important:

timerTest.ino: In member function ‘virtual void test_timer_delayToNextEvent::once()’:
timerTest.ino:295:22: warning: comparison of integer expressions of different signedness: ‘long unsigned int’ and ‘int’ [-Wsign-compare]

Ensure that anything getting or comparing values from simMillis matches its return type unsigned long. Same with the ticks and tick return types. Probably easiest to just use unsigned long for everything instead of int.

@philj404
Copy link
Contributor Author

philj404 commented Nov 5, 2020

Done.

  • I'm using unsigned long more consistently now.
  • I also set the Makefile to stop the build on warnings.

Note these changes will now be less forgiving than just running your tests on an Arduino Uno. It's a different build environment with different default warnings.

Contributors writing unit tests should probably use a Linux/MacOS system and clone UnixHostDuino into Arduino/libraries.

While technically the GitHub Action will return error messages, it's an awkward feedback loop compared to just doing the (Linux) build/test cycle locally.

@contrem
Copy link
Owner

contrem commented Nov 5, 2020

Don't forget your constructor arguments and class members:

    DummyTask(int runTime, bool repeats = true):
      busyTime(runTime), repeat(repeats)
    {
      reset();
    };

    int busyTime;
    int numRuns;
    int timeOfLastRun;

Could you explain your choice to use assertNear here, and not check for an absolute value? It's a fixed test, the value shouldn't change or fluctuate.

  assertNear(dt_3millisec.numRuns, 100, 9); // 7+ millisecs apart
  // (ideally 142 runs)

  assertNear(dt_5millisec.numRuns, 90, 4); // 11+ millisecs apart
  // (ideally 90 runs)

@philj404
Copy link
Contributor Author

philj404 commented Nov 6, 2020

Thanks for reviewing my code at this level of detail. Yes those member variables should be unsigned long too -- although the test is over before it can notice.

There are a couple reasons I prefer to use assertNear() for counting the number of times a task ran.

  1. I did not mathematically prove what the correct counts were for when these tasks interfere with each other.
  2. Originally the test used delay() and millis() directly. I did not want the test to fail just because a processor was busy or extra-slow (on an arbitrary platform -- which might even be a virtual machine now).
  3. I do not want to over-constrain the timer requirements. The timer's also a scheduler. I do want to ensure that no task starves. But it's OK for the test if the timer code changes to check the queue in reverse order, or do something with priority queues... which might change the number of runs slightly.
  4. Right now I'm more interested in getting the unit test infrastructure working correctly.

TimerTest is more a smoke-test or proof-of-principle. It's a good start (and hopefully confirms correct behavior as far as it goes). It is not a thorough exercise of all arduino-timer features. But there should be enough of an example here to show what to do to add REAL tests you want.

@contrem
Copy link
Owner

contrem commented Nov 6, 2020

Tracking in pj/ci-unit-tests.

Please close the other PR if this one contains it.

@philj404 philj404 mentioned this pull request Nov 6, 2020
@contrem
Copy link
Owner

contrem commented Nov 7, 2020

Merged in e12c01a.

@philj404
Copy link
Contributor Author

philj404 commented Nov 8, 2020

Thanks for doing this merge! It appears there may be an indentation conflict with default Arduino settings... which are not self-consistent anyway:

  • The default auto-format indent size is 2. (Set in .../formatter.conf as indent=spaces=2 .)
  • The indent size this library prefers is 4. (This is also the default Arduino tab setting. Set in .../preferences.txt as editor.tabs.size=2 .)

So... sorry if my contributions weren't quite conformant to your library's standards. I will be more careful next time.

That said, it looks like this pull request has been merged in with appropriate formatting. It is ready to close if you think you are done too.

@contrem
Copy link
Owner

contrem commented Nov 9, 2020

Thanks for the info on setting the Arduino indentation settings, and thanks for helping improve the library.

@contrem contrem closed this Nov 9, 2020
@philj404 philj404 deleted the feature/ciUnitTests branch March 2, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants