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

WIP: Fixing unittests #435

Draft
wants to merge 31 commits into
base: master
Choose a base branch
from
Draft

WIP: Fixing unittests #435

wants to merge 31 commits into from

Conversation

Igorshp
Copy link
Contributor

@Igorshp Igorshp commented Dec 12, 2020

Trying to get the testsuite to run.

Will be updating this PR over time.

Help is much appreciated.

To run the tests:

make test

you may need to have clang, llvm and libblocksruntime-dev installed

Status

  • ✅ alignsensor_unittest.cc
  • ✅ arming_prevention_unittest.cc
  • ✅ atomic_unittest.cc
  • ✅ baro_bmp280_unittest.cc
  • ✅ baro_ms5611_unittest.cc
  • ✅ blackbox_encoding_unittest.cc
  • ✅ blackbox_unittest.cc
  • ✅ cli_unittest.cc
  • ✅ cms_unittest.cc
  • ✅ common_filter_unittest.cc
  • ✅ encoding_unittest.cc
  • ✅ flight_failsafe_unittest.cc
  • ✅ flight_imu_unittest.cc
  • ✅ gps_conversion_unittest.cc
  • ✅ huffman_unittest.cc
  • ✅ io_serial_unittest.cc
  • ✅ ledstrip_unittest.cc
  • ✅ maths_unittest.cc
  • ✅ osd_unittest.cc
  • ✅ pg_unittest.cc
  • ✅ pid_unittest.cc (test itself fails ❌ )
  • ✅ rc_controls_unittest.cc
  • ✅ rcdevice_unittest.cc
  • ✅ rx_crsf_unittest.cc (test itself fails ❌ )
  • ✅ rx_ibus_unittest.cc
  • ✅ rx_ranges_unittest.cc
  • ✅ rx_rx_unittest.cc
  • ✅ scheduler_unittest.cc
  • ❌ sensor_gyro_unittest.cc (requires arm_math.h library)
  • ✅ telemetry_crsf_msp_unittest.cc
  • ✅ telemetry_crsf_unittest.cc (test itself fails ❌ )
  • ✅ telemetry_hott_unittest.cc
  • ✅ telemetry_ibus_unittest.cc
  • ✅ transponder_ir_unittest.cc
  • ✅ vtx_unittest.cc (fixed in enhancement: CLI: allow vtx power to be changed by a switch #417, now merged)
  • ✅ ws2811_unittest.cc

❌ Tests running in github action step (currently failing to compile)

@KAJE1977
Copy link

unittest.txt

@nerdCopter nerdCopter marked this pull request as draft December 13, 2020 18:15
@gretel
Copy link
Contributor

gretel commented Dec 13, 2020

@Igorshp cool!

@Igorshp Igorshp force-pushed the unittests branch 8 times, most recently from dc9027a to 0674e83 Compare December 16, 2020 02:11
@Igorshp
Copy link
Contributor Author

Igorshp commented Dec 16, 2020

Pretty much all compilation errors resolved.

having trouble with sensor_gyro_unittest.cc, it complains about missing arm_math.h, I belive it could be because i'm compiling it on x86 architecture. Not sure.

Some tests are also failing, expected as the code has diverged since.

@gretel
Copy link
Contributor

gretel commented Dec 16, 2020

@Igorshp the file is at ./lib/main/CMSIS/DSP/Include/arm_math.h but i think it has sth to do with the target supporting SMT or not.

@Igorshp
Copy link
Contributor Author

Igorshp commented Dec 16, 2020

@Igorshp the file is at ./lib/main/CMSIS/DSP/Include/arm_math.h but i think it has sth to do with the target supporting SMT or not.

hmm, intresting point. Thanks. I'll check it can be toggled with flags

@nerdCopter
Copy link
Member

nerdCopter and others added 2 commits December 17, 2020 14:30
* buildtest branches too
* minor artifact name fix
* workflow bash([[ == ]]), not posix([ = ]), nor broken mix.
```report_cell_voltage``` is not used currently for crossfire. This request fixes it.
@Igorshp
Copy link
Contributor Author

Igorshp commented Dec 20, 2020

Status update

All but one tests compile (sensor_gyro_unittest requires arm_math.h library and I haven't yet figured out how to provide it)

3 tests compile but are failing since code base have moved on.

The github action step fails to run the tests. Looks like environment setup issue.

@gretel
Copy link
Contributor

gretel commented Dec 20, 2020

@Igorshp thanks!

The github action step fails to run the tests. Looks like environment setup issue.

it's a compiler issue - the action will be fine. the current state of the affair is like this:

we should just wait a couple of days for the upstreams to settle. 🍵

@gretel
Copy link
Contributor

gretel commented Dec 20, 2020

ok got the test to run to completion. credits to @mikeller 😗

@Igorshp now, i can either PR this up as new or you rebase my branch on your open PR.. i'm fine both ways.

@Quick-Flash the tests show some interesting results including the pid controller 😼

@Igorshp
Copy link
Contributor Author

Igorshp commented Dec 20, 2020

@gretel interesting, thanks for the insight!

I've had a fair share of issues with the compiler myself and considering building a docker container to run the tests in. It'll help normalize the testing environment for everyone and from what I understand github actions can use containers.
any thoughts?

Then there's matter of broken tests. crsf ones dont look to bad. pid controller stuff though is a bit out of my league at the moment.

@gretel
Copy link
Contributor

gretel commented Dec 20, 2020

I've had a fair share of issues with the compiler myself and considering building a docker container to run the tests in. It'll help normalize the testing environment for everyone and from what I understand github actions can use containers.
any thoughts?

no more docker please 🙈 i think it's working fine now.

Then there's matter of broken tests. crsf ones dont look to bad. pid controller stuff though is a bit out of my league at the moment.

there is also the matter of broken implemenations 👯

gretel and others added 2 commits December 20, 2020 05:17
…light#10265; backport of betaflight/betaflight#10288; correction of issues showing up using the betaflights improvements; fix unusued variable in 'crsf.c' to get the test to run to completion
@Igorshp
Copy link
Contributor Author

Igorshp commented Dec 20, 2020

Merged in your changes, let's wait for the build

EDIT: hmm, couple of other commits got pulled in. I'll rebase on master when we're done

@gretel
Copy link
Contributor

gretel commented Dec 20, 2020

i think the Take the trash out out step is also "required" for the test run.

@nerdCopter
Copy link
Member

raw logs:
logs_1229.zip
test: Error: Process completed with exit code 2.

@nerdCopter
Copy link
Member

Tried getting this to work with clang-11 and/or clang-12 for modern OS.
easy to change src/test/Makefile, but DWARF/ld errors occur throughout.

[  PASSED  ] 8 tests.
running test_alignsensor_unittest: PASS
linking ../../obj/test/arming_prevention_unittest/arming_prevention_unittest 
/usr/bin/ld: /usr/bin/ld: DWARF error: could not find variable specification at offset 38fb
/usr/bin/ld: DWARF error: could not find variable specification at offset 5979
/usr/bin/ld: DWARF error: could not find variable specification at offset 5a1e
/usr/bin/ld: DWARF error: could not find variable specification at offset 5ac4
/usr/bin/ld: DWARF error: could not find variable specification at offset 5b6f
/usr/bin/ld: DWARF error: could not find variable specification at offset 5c1a
/usr/bin/ld: DWARF error: could not find variable specification at offset 5cc5
/usr/bin/ld: DWARF error: could not find variable specification at offset 5d70
/usr/bin/ld: DWARF error: could not find variable specification at offset 5e1b
../../obj/test/arming_prevention_unittest/arming_prevention_unittest.o:./src/test/../main/flight/servos.h:44: multiple definition of `inputSource_e'; ../../obj/test/arming_prevention_unittest/fc/fc_core.c.o:./src/test/../main/flight/servos.h:44: first defined here
/usr/bin/ld: ../../obj/test/arming_prevention_unittest/arming_prevention_unittest.o:./src/test/unit/arming_prevention_unittest.cc:58: multiple definition of `rcCommand'; ../../obj/test/arming_prevention_unittest/fc/rc_controls.c.o:./src/test/../main/fc/rc_controls.c:73: first defined here
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [Makefile:664: ../../obj/test/arming_prevention_unittest/arming_prevention_unittest] Error 1
make[1]: Leaving directory './src/test'
make: *** [Makefile:540: test] Error 2

if this can be fixed, then master merged in, it will need an #ifdef for SmithPredictor

--- a/src/main/sensors/gyro.h
+++ b/src/main/sensors/gyro.h
@@ -195,7 +195,9 @@ bool gyroOverflowDetected(void);
 bool gyroYawSpinDetected(void);
 uint16_t gyroAbsRateDps(int axis);
 uint8_t gyroReadRegister(uint8_t whichSensor, uint8_t reg);
+#ifdef USE_SMITH_PREDICTOR
 float applySmithPredictor(smithPredictor_t *smithPredictor, float gyroFiltered);
+#endif

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.

6 participants