Create NodeMCU test system NTest based on gambiarra

[HHHartmann]

helps fixing #2145

• This PR is for the dev branch rather than for master.
• This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• I have thoroughly tested my contribution.
• The changes are reflected in the documentation.

this has evolved, so I changed this description too:
Add test framework which can run tests like the following on an ESP

N.testasync('SEMI alarm', function(next)
  local t = tmr.create();
  local count = 0
  t:alarm(200, tmr.ALARM_SEMI, 
    function(tp)
      count = count + 1
      if count <= 1 then
        tp:start()
        return
      end
      ok(eq(count, 2), "only 2 invocations")
      next()
    end)
  ok(true, "sync end")
end)

Also includes first tests for the file and tmr module, mainly to have a working example.

[HHHartmann]

NOTE: The readme is not yet updated.

[nwf]

It'd be really nice to be able to announce the existence of our test framework by the next release. @HHHartmann have you had time to weigh the pros and cons of mispec and gambiarra? Would you mind doing a bit of compare-and-contrast for those of us not familiar?

[nwf]

@HHHartmann Would you be opposed to just using tests/ instead of lua_tests/? I find myself wanting to write a bunch of HW tests in Lua using the as-yet-still-being-defined "NodeMCU Test Environment", which I think most naturally belongs next to the sea of TCL, which seems more like a tests/ and less like a lua_tests/ kind of thing.

[nwf]

Having played a little with gambiarra now, I think I like it, except that we don't know how many tests we're going to run, which makes things a little sad from TAP's perspective. We could require the tests themselves to generate the TAP plan message of "TAP 1..34" (e.g.) (which might be a perfectly OK thing to do, and is in fact kind of typical of TAP-based frameworks; it's just that because myspec build up the whole test table up front, it was easy to count).

[HHHartmann]

I am also in favor of gambiarra. I already added some more functionality to it and there might be more.
It also came with tests to test itself, so refactoring and adding is safer.
The syntax is also clearer than in mispec.

I am also working on first async tests which I will commit soon.

[HHHartmann]

I don't mind renaming the directory. Just thought that it matches the remaining directories containing lua code.
But as a non native speaker I don't get all of the sentence. What is TCL? I often struggle with acronyms you seem to like.

[nwf]

Sorry to be dense in communication. FWIW, your English seems better than mine, and I'm a monolingual dunce. ;)

TCL is the Tool Command Language and is used by expect (https://core.tcl-lang.org/expect/index), a program for automated control of other programs via text (usually their stdin/stdout). See the .expect files in https://github.com/nwf/nodemcu-firmware/tree/dev-active/tests and the library .tcl files in https://github.com/nwf/nodemcu-firmware/tree/dev-active/tests/expectnmcu .

I think (but I'm open to suggestions) that it makes sense to put all our core tests in a single directory, regardless of their implementation language, and I, lacking creativity, chose tests/. There's no real harm in a tests/ and lua_tests/ split, it just feels a little odd to me.

Case in point: I want to write (in Lua, and probably gambiarra at that, rather than expect) a test of the adc module. To do this, I was going to depend on hardware as defined in https://github.com/nwf/nodemcu-firmware/blob/dev-active/tests/NodeMCU_Test_Environment.rst and so despite the implementation language, tests/test-adc.lua seemed more of the right place than lua_tests/test-adc.lua or such.

[HHHartmann]

Wow your test plans are taking shape. As I see it the plan with the teo ESPs is that they are only needed for extended perifery testing. I would like to also use them for tcp testing. One being the testee and one being the known-to-work counterpart which is also steered by the test running on the first ESP.

But back to the naming issue: If this folder is to be populated by other test files it does not make sense to name it "lua" something of course.
May be we should have subfolders for files which are for execution on (one of) the ESP8266 and the ones for the host.

[nwf]

test-net-intermodule.expect is something of a placeholder right now; yes, it really should be doing packet exchange between the two.

[HHHartmann]

I think it would be better to have expect only orchestrate the test runs and execute the tests themselves as a Lua script on the ESP. That way everyone wanting to run a test could just do that without having to set up expect at all.
Also not having to learn another language to do testing would sure help developers to accept that they have to write tests.
We should really keep the hurdle as low as possible here.

[nwf]

I really, truly appreciate your point. I think the vast majority of testing can happen on device, which is why I wrote https://github.com/nwf/nodemcu-firmware/blob/dev-active/tests/tap-driver.expect and https://github.com/nwf/nodemcu-firmware/blob/dev-active/tests/shim_gambiarra.lua and nwf@28fd42e . Things like adc and i2c and gpio and encoder and crypto and pixbuf and so on can merrily live 100% in Lua and run on device and just be orchestrated, as you say, by programs on the host. (The use of TAP, in fact, means that it should be easy to not use expect in some eventuality. It's designed, on purpose, to be language-agnostic.) I just haven't written many examples yet, but that absolutely is the goal. I guess I can point at https://github.com/nwf/nodemcu-firmware/blob/dev-active/lua_modules/liquidcrystal/test-i2c4bit.lua and the converted https://github.com/nwf/nodemcu-firmware/blob/dev-active/lua_tests/mispec_pixbuf_1.lua if you want proof I'm not BSing when I say that I think most testing can be on the device.

But certain things (esp., the trickier modules, which are all I've written tests for, because the easy ones are easy and I know how they will work, once I know what test harness we'd like to use) like net and tls and mqtt and uart and so on need orchestration not just on one NodeMCU device, but on either two NodeMCU devices or on a NodeMCU device and programs running on a host computer. In some cases there truly isn't any getting around that (tls cannot connect to another NodeMCU device and mqtt needs a server) while in some cases the increased power of orchestration on the host means that our tests have shorter dependencies (uart and net-intermodule can test cross-device without needing two independent cross-device signaling mechanisms). There isn't, so far as I know, a suitable in-Lua replacement for the power of expect, which is why I've written those tests in TCL.

[nwf]

Well, perhaps rather than all those words, an example is in order. Here, https://github.com/nwf/nodemcu-firmware/blob/dev-active/tests/test-adc.lua, is a test of the adc module. I think it conveys the flavor of how I expect on-device tests to look. It can be run using the tap-driver.expect program, paired with an LFS image containing gambiarra, like this:

TCLLIBPATH=./expectnmcu expect ./tap-driver.expect -serial /dev/ttyUSB0 -lfs ./lfs.out ./test-adc.lua

If the LFS also contains test-adc.lua, there's no need to transfer it, so the -noxfer flag can be used to speed things up.

I'm worried that gambiarra doesn't use node.task.post(), so the whole program runs without any return to the SDK. ISTR this was not true of mispec but perhaps is also what you were alluding to with your coroutine comment on #2145?

[HHHartmann]

The test looks very nice. Especially the generation of testcases. Maybe we could add data driven tests here, Just giving an array of params and then integrate the loop into gambiarra.

But for the node.task.post() issue. Your assumptions are correct. The file test takes several seconds without rebooting the device. Probably due to no tcp activity during that time.

But it should be no problem instrumenting gambiarra with that. It supports async tests allready.
It the also could print the number of tests up front.

About the coroutines:
Currently tests look like this:

test('timer real async', function(next)
  local t = tmr.create();
  t:alarm(500, tmr.ALARM_SINGLE, function() 
             ok(true, "async end")
             next()
        end)
  ok(true, "sync ende")
end, true)

would change to something like this:

test('timer real async', function(CallbackFactory, await)
  local t = tmr.create();
  t:alarm(500, tmr.ALARM_SINGLE, CallbackFactory.timeout)
  ok(true, "sync ende")
  cbName, param1 = await()
  ok(eq("timeout", cbName))
  -- param1 is the timer which has been passed to the callback
  ok(true, "async end")
end, true)

So especially for simpler tests this seems easier to code to me.
The test just ends at the end of the coroutine.

for fans of the next methode to be called to indicate the end of the test this can be used:

test('timer real async', function(CallbackFactory, await)
  next = CallbackFactory.next
  local t = tmr.create();
  t:alarm(500, tmr.ALARM_SINGLE, function() 
             ok(true, "async end")
             next()
        end)
  ok(true, "sync ende")
  await()
end, true)

The while test runs in a coroutine and the await switches contect and waits for any callback issued by the factory and the returns wizth the first param as the callback name and the others the parameters passed to the cb.

[nwf]

I think I misunderstand the gambiarra interface; is the intent to call test once and use ok and ko and friends internally, or is it expected that the caller will make repeated calls to test?

Your examples seem like they have a good bit of boilerplate that's required for the async/callback machinery to operate; could we somehow more directly encapsulate that inside gambiarra? Say, by making ok and friends yield through the user-provided function back to test?

[HHHartmann]

The idea is to make repeated calls to test.

I think it would be a good idea to start each test with a post but it would break async testing when a callback could happen on every ok and nok. Just imagine executing an ok and a callback altering the data being checked. That would be a race condition causing the test to fail randomly.
It would however make sense to allow a timeout to the await function. So if the test requires it an await(0) could be issued.
I think that running all tests as async (inside a coroutine) does not make sense as it requires extra memory.

Also a failing ok or nok should fail the current test and not continue as it is now.

[nwf]

I'm concerned that the async tests that call next leave the test function on the call stack... that could, maybe, be return next() instead, making it a tail call, but even then, I'd want to be convinced that we aren't growing the call stack with a series of async tests.

[nwf]

Hm. Mixing async and sync tests seems more subtle than I'd want. I am trying to test gpio.trig with

test('gpio toggle trigger 1', function(next)
  seti2cb7(false)
  tmr.delay(100)
  gpio.trig(8, "both", function(l,w,c)
    print("TRIG 1", l, w, c)
    ok(c == 1 and l == 1)
    return next()
  end)
  seti2cb7(true)
end, true)

test('gpio toggle trigger 2', function(next)
  gpio.trig(8, "both", function(l,w,c)
    print("TRIG 2", l, w, c)
    ok(c == 1 and l == 0)
    return next()
  end)
  seti2cb7(false)
end, true)

test('gpio toggle trigger end', function(next)
  gpio.trig(8, "none")
  ok(true)
  return next()
end, true)

and that works if it's the end of the file, but if I replace the gpio toggle trigger end test with

test('gpio toggle trigger end', function()
  gpio.trig(8, "none")
  ok(true)
end)

then that one runs as soon as gpio toggle trigger 1 yields, which isn't ideal (and, indeed, prevents the callback from firing). I guess I understand why that has to be true, since gambiarra does not collect all the tests before running them but it's unfortunate.

ETA: My default response to this would be to avoid on-device testing of anything asynchronous and just go hide behind the big hammers I know well (that is, expect) but I am open to the idea that I'm not handling the on-device code correctly.

[HHHartmann]

Good to see, that you are having a look at this. I also stumbled across the fact, that the gambiarra_test file contains async tests only at the end but didn't have a closer look yet.
As you mentioned above gambiarra does not use a task.post to start the tests. Adding this would have two benefits.

1. the function would have a chance to get popped off the callstack after it executed
2. All tests would be stored in a list before executing them, regardless of async or not and get executed in the original order, one by one. That way a testrun would first execute the main program to it's end and then start the tests. This would also allow for TAP to know in advance how many tests to expect (well a statement after the last test would need to be inserted.)

I am currently fixing some other stuff and changing the ok and nok tests to abort the test instead of continuing it.

After that I will add starting the tests with a post rather than directly. So everything will be asynchronous in the end.

Currently I am having problems if a test tails in a callback with a Lua error. They just reboot instead of failing the test. But maybe I can catch those with the new errorhandler which Terry iplemented. Will have to think about that next.
Maybe using the coroutines as I mentioned above could help fixing that too.

[HHHartmann]

Added support for sync test after async and evicting old tests from callstack.
So the following code now produces expected results:

local test = require('gambiarra')

local marker = 0
test('set marker async', function(next)
  local t = tmr.create();
  t:alarm(500, tmr.ALARM_SINGLE, function() 
    marker = "marked"
    ok(true, 'async bar')
    next()
  end)
  ok(true, 'foo')
end, true)

test('check marker sync', function()
  ok(eq(marker, "marked"), "marker check")
end)

test('check marker async', function(next)
  ok(eq(marker, "marked"), "marker check")
  next()
end, true)

[HHHartmann]

Added catching erros and failed tests in callbacks also.
I am using the brand new node.setonerror functionality for this.

Next will be the integration of coroutines to run async tests as explained in #2984 (comment)

I have it running but need to integrate it in gambiarra. Or should it stay separated?

[HHHartmann]

@nwf I would like to change the interface of gambiarra to allow for separate methods for e.g. setting the output handler and also add the ability to define after-test cleanup routines and before-test prepare routines.
I'd like to hear your or everybody elses opinion on this.

the interface would then be somethink like this:

- local test = require('gambiarra')
+ local gambiarra = require('gambiarra')
+ local test = gambiarra.test
+ gambiarra.test("name", fn)
+ gambiarra.asynctest("name", fn)
+ gambiarra.cotest("name", fn)    -- the yet to come tests, that run in coroutines

function report(e, test, msg, errormsg)
  if e == 'begin' then
...
end
--  setting the report function, replacing the default one
- test(report)
+ gambiarra.report= report

gambiarra.cleanup = function() ... end
gambiarra.prepare = function() ... end

cleanup and prepare can be set as often as needed and will be valid for all tests thereafter.
Especially the cleanup part is important to clean up async tests which have pending callbacks. Imagine a tmr test failing and the timer still being armed.

[nwf]

That seems like a quite reasonable API. A few thoughts:

Any reason not to run every test through coroutine.resume()? Are coroutines RAM-heavy? (I've only used them in "real computer" Lua, so heap wasn't really a concern, there.)

Given the "parse entire file into one chunk" nature of Lua's loadfile/dofile (rather than "parse file as if it were fed to the REPL"), I wonder if it's worth pondering how to simplify spreading tests across several files? Maybe this is already sufficiently simple that it needs no new support, but it could be nice to have a worked example.

Nit: I'd prefer gambiarra to not be a singleton (i.e. gambiarra = (require "gambiarra").new() rather than just require "gambiarra") so that in principle we could run several sets of tests without rebooting or having to explicitly reset the gambiarra state.

[nwf]

One extremely minor thing I noticed while reading through. If you like it as it is, I'm content to leave well enough alone.

Once again I have to thank you for all your hard work on this; NTest looks really nice.

[nwf]

Is this ready to (squash) merge?

[HHHartmann]

From my point of view it is ready. I'v got more ideas but that's for a later PR. First we should get some experience with this.
And Thank you for all the good feedback and ideas you had on this and your continued review and testing.

[marcelstoer]

The /tests folder probably needs a README or index file as to get a testing overview if you navigate to the folder on GitHub? Also, maybe mention somewhere what to do about /lua_tests?

[marcelstoer]

That is a dead end. Maybe link to https://github.com/imolein/gambiarra instead?

[HHHartmann]

The source you give is also based on zserges gambiarra. zserge seems to have removed the repo, so I would just link to his account https://bitbucket.org/zserge
But the repo you give has a much better readme, so we might copy some from there.

[HHHartmann]

The /tests folder probably needs a README or index file as to get a testing overview if you navigate to the folder on GitHub? Also, maybe mention somewhere what to do about /lua_tests?

The 'plan' was to convert them to NTest but as #3158 is on the way I was hoping that it would be fixed therein.
But I can convert them before that too.

[pjsg]

What is unclear to me is how to actually run the tests -- are there step-by-step instructions somewhere?

[nwf]

So far as I know there are no step-by-step instructions. Eventually, I hope the answer to be a shell script on the host controlling the DUTs, and I have such a thing, more or less, somewhere between my desk and my dev-active branch (especially the tap-driver.expect program), but at the moment I think the answer is "build a LFS containing NTest and the tests you want, flash it, and then run them like any other Lua program".