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

2.3.0 release #73

Closed
wmacevoy opened this issue Mar 18, 2018 · 17 comments
Closed

2.3.0 release #73

wmacevoy opened this issue Mar 18, 2018 · 17 comments

Comments

@wmacevoy
Copy link
Collaborator

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
Copy link

bxparks commented Mar 19, 2018

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
Copy link

bxparks commented Mar 19, 2018

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@bxparks
Copy link

bxparks commented Mar 22, 2018

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

  • AVR: ArduinoUnit (33790/1059); AUnit (18928/917)
  • Teensy LC: ArduinoUnit (35232/2980); AUnit (26496/2780)
  • ESP8266: ArduinoUnit (276264/35140); AUnit (268236/33128)

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
Copy link
Collaborator Author

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

@wmacevoy
Copy link
Collaborator Author

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

@wmacevoy
Copy link
Collaborator Author

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
Copy link

bxparks commented Mar 27, 2018

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

wmacevoy commented Apr 3, 2018

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
Copy link

bxparks commented Apr 4, 2018

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
Copy link
Collaborator Author

wmacevoy commented Apr 4, 2018 via email

@wmacevoy
Copy link
Collaborator Author

wmacevoy commented Apr 4, 2018

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
Copy link

bxparks commented Apr 4, 2018

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
Copy link
Collaborator Author

wmacevoy commented May 3, 2018

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

@wmacevoy wmacevoy closed this as completed May 3, 2018
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

No branches or pull requests

2 participants