mmurdoch / arduinounit

ArduinoUnit is a unit testing framework for Arduino libraries
MIT License
394 stars 51 forks source link

2.3.0 release #73

Closed wmacevoy closed 6 years ago

wmacevoy commented 6 years ago

There have been some long-standing feature requests; mostly support for ESP 8266 and ESP 32 boards, and a nagging problem with duplicate strings in FLASH. This version DROPS some old compiler support features, and adds ESP 8266, ESP 32, and flash string duplication, along with some refactoring to shrink the footprint of the testing env.

It is in alpha - please try it and report issues. v2.3.0-alpha

bxparks commented 6 years ago

Compiling using v2.3.0-alpha tag, on ESP8266, I get:

/Users/brian/dev/arduino/libraries/ArduinoUnit/src/ArduinoUnitUtility/Flash.h:8:40: fatal error: cores/esp8266/pgmspace.h: No such file or directory
 #    include <cores/esp8266/pgmspace.h>
                                        ^
compilation terminated.

I changed Flash.h to be:

#  if defined(ESP8266)
#    include <pgmspace.h>
#  elif defined(ESP32)
...

Recompiling, I then get:

In file included from /Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/Arduino.h:244:0,
                 from sketch/AceButtonTest.ino.cpp:1:
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/pgmspace.h:16:51: error: __c causes a section type conflict with __c
 #define PSTR(s) (__extension__({static const char __c[] PROGMEM = (s); &__c[0];}))
                                                   ^
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/WString.h:38:76: note: in definition of macro 'FPSTR'
 #define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer))
                                                                            ^
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/WString.h:39:34: note: in expansion of macro 'PSTR'
 #define F(string_literal) (FPSTR(PSTR(string_literal)))
                                  ^
/Users/brian/dev/arduino/libraries/ArduinoUnit/src/ArduinoUnit.h:538:152: note: in expansion of macro 'F'
 #define assertOp(arg1,op,op_name,arg2) do { if (!Test::assertion<__typeof__(arg1),__typeof__(arg2)>(F(__FILE__),__LINE__,F(#arg1),(arg1),F(op_name),op,F(#arg2),(arg2))) return; } while (0)
                                                                                                                                                        ^
/Users/brian/dev/arduino/libraries/ArduinoUnit/src/ArduinoUnit.h:541:38: note: in expansion of macro 'assertOp'
 #define assertEqual(arg1,arg2)       assertOp(arg1,compareEqual,"==",arg2)
                                      ^
AceButtonTest.ino:1386:3: note: in expansion of macro 'assertEqual'
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/pgmspace.h:16:51: note: '__c' was declared here
 #define PSTR(s) (__extension__({static const char __c[] PROGMEM = (s); &__c[0];}))
                                                   ^
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/WString.h:38:76: note: in definition of macro 'FPSTR'
 #define FPSTR(pstr_pointer) (reinterpret_cast<const __FlashStringHelper *>(pstr_pointer))
                                                                            ^
/Users/brian/Library/Arduino15/packages/esp8266/hardware/esp8266/2.4.0/cores/esp8266/WString.h:39:34: note: in expansion of macro 'PSTR'
 #define F(string_literal) (FPSTR(PSTR(string_literal)))
                                  ^
/Users/brian/dev/arduino/libraries/ArduinoUnit/src/ArduinoUnit.h:517:81: note: in expansion of macro 'F'
 #define test(name) struct test_ ## name : TestOnce { test_ ## name() : TestOnce(F(#name)) {}; void once(); } test_ ## name ## _instance; void test_ ## name :: once() 

I'm curious, did you try compiling this for the ESP8266? Maybe you forgot, but you don't actually need to have an ESP8266 microcontroller to compile. Just add the ESP8266 support using the Board Manager in the Arduino IDE, as described here: https://arduino-esp8266.readthedocs.io/en/2.4.1/installing.html Then select one of the ESP boards (NodeMCU 1.0 is a good one), and hit "Verify/Compile".

bxparks commented 6 years ago

The cause of the dreaded error: __c causes a section type conflict with __c is that your #define test() macro uses an inlined constructor. In the ESP8266 compiler, F() strings (i.e. PROGMEM strings) cannot be in both a function context and an inlined context because the compiler tries to place the strings into 2 different .data segments, and it gets confused (or something like that, I'm paraphrasing my limited understanding of how GCC lays out the flash memory strings).

You can see how I solved this problem in AUnit/Test.h, by pulling out the constructor so that it's no longer inlined, then making sure that it's in a function context, by placing the F() string into constructor initialization of the parent class, instead of chaining it into the constructor argument of the base class (which causes the same error as above).

wmacevoy commented 6 years ago

Ok, no, I did not think of that. It also broke everything. I reduced F() to AVR only (the esp's have lots of ram in any case), and fiddled away. It now compiles on the targets. ... Now can you try to run the firmware on an esp and see if it passes? Just follow the menu when it boots...

wmacevoy commented 6 years ago

Sorry -- please pull the latest from #iss73 branch.

bxparks commented 6 years ago

Looks like iss73 branch works on ESP8266. The numbers I get are (flash/static) in bytes:

Am I correct in assuming that you don't actually have an ESP8266? If you intend to support this microcontroller in ArduinoUnit, it might be worth getting a couple of them because the testing cycle will be a lot faster if you ran the tests yourself. They are cheap as heck (e.g. $13.99 for 2 on Amazon) for one that uses the CP2102 usb-serial chip.

wmacevoy commented 6 years ago

Correct. It isn't a matter of price, but of choosing what to support. Thanks for running it.

wmacevoy commented 6 years ago

I bumped to 2.3.1-alpha due to number of changes...

wmacevoy commented 6 years ago

Ok the latest of this branch has the following quite useful new features:

  1. ESP8266/ESP32 support.
  2. Less memory (AVR flash strings are de-duplicated, other platforms use RAM).
  3. Optional output with asserts (evaluated lazily, so it's not wasting resources).
  4. Builds in a standard GCC/LLVM environment (for en vitro unit tests).
bxparks commented 6 years ago

I noticed this little remark in the release notes for v2.3.2-alpha:

Added optional messages to all asserts (optional last parameters), as in assertEquals(x,y,"woot " << x << " is not " << y << "here!") The optional text appears in [...] at the end of the assert message.

I couldn't find a feature request issue for this, so I'll comment here: Did you consider following the syntax used by Google Test, so that people who are familiar that framework don't have to remember a new syntax?

assertEquals(x, y) << "woot " << x << " is not " << y << "here!";
wmacevoy commented 6 years ago

There are good reasons. The au version builds a lambda function which is only invoked if output is necessary. The only way I can think to make the google version work is to format all the data every time and send that data to a null stream if not needed. Assuming most tests silently pass, the google (and junit version, btw) is roughly infinitely more expensive.

wmacevoy commented 6 years ago

This re-work just removed a bunch of code - Compare is now small and does not have to be automatically generated. Same features; less filling!

wmacevoy commented 6 years ago

Ok, just added mocking with proper licensing that allows for arduino unit testing on dev (vs target) system; look at "en vitro" in README.md. This concludes changes to the library proper for this release up to bug fixes. The only feature change, which will not effect the library proper, may be supporting continuous integration.

bxparks commented 6 years ago

Great work on the cleanup. Just having a simpler Compare.h is worth it, the old version was basically unreadable and unmaintainable.

With regards to the streaming expression inside the the assertXxx(), I think your design decision is appropriate. Just a small correction though, that Google Test only evaluates the additional message if the assertion fails. From my reading of the code, it's a null operation when the assertion succeeds. For JUnit, you are correct, the String is fully evaluated, and most people use a String.format() there. I wish the designer of JUnit had placed the error message at the end of the assertXxx(), and added a vararg.

I haven't followed all of your recent changes, there's been so many. But I did notice a bug that carries over to the new version, so I'll open up a ticket for that. I wanted to wait until things settled down before bring it up.

Again, nice work on the new version.

wmacevoy commented 6 years ago

How can google test not evaluate << expensive() with that notation? (I agree is does not print out the value, but expensive needs to be calculated and discarded each time, no?) [and formatting almost always is far more expensive than testing]

wmacevoy commented 6 years ago

Ok. I'm beginning to see the light here. It would be tricky to get it into the footnote [ ] notation as currently advertised. But I see how GoogleTest might pull that off without evaluating expensive every time.

bxparks commented 6 years ago

I assume that we're both looking at this fragment in gtest_pred_impl.h:

#define GTEST_ASSERT_(expression, on_failure) \
  GTEST_AMBIGUOUS_ELSE_BLOCKER_ \
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
    ; \
  else \
    on_failure(gtest_ar.failure_message())

If I understand Google Test correctly, their implementation has two disadvantages compared to ArduinoUnit:

  1. it cannot print success assertXxx() messages
  2. it cannot return early from the current method if an assertion fails

I actually prefer ArduinoUnit's implementation. I've found the success messages very helpful for debugging complex, custom assertion methods. And the early return seems far cleaner to me, and again supports custom assertion methods better (well, at least in AUnit where I've taken advantage of it :-)).

wmacevoy commented 6 years ago

Ok v3.0 is now released! May your tests be bountiful & systems reliable!