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

unittests: remove blacklisting for non-32 platforms #12447

Closed
wants to merge 3 commits into from

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 14, 2019

Contribution description

Inspired by #9081 I looked into the unittests what could be cleaned up there to remove BOARD_BLACKLIST. For this I had to:

  • Adapt some test values to make the tests compilable for non-32-bit platforms again
    • for tests-pkt: it just compares the sizes, so the value is arbitrary
      and can be cast to size_t without worries
    • for tests-rtc: 360 and 120 are the same under mod 60 (which seconds usually are), but 120 fits into int8_t's value space used by avr-libc's struct tm.
  • blacklist arch_msp430 for the tests-rtc test suite, as it uses mktime() which is not provided by msp430-libc

Testing procedure

tests/unittests should still work. Specifically, they should now also work (test-suite-wise) on AVR-based platforms and for the most part on MSP430-based (excluding the tests-rtc test suite).

Issues/PRs references

Follow-up to #9081, reverting parts of #12040.

This makes most unittests compilable for non-32-bit platforms again:
- for `tests-pkt`: it just compares the sizes, so the value is
  arbitrary and can be cast to `size_t` without worries
- for `tests-rtc`: 360 and 120 are the same under `mod 60` (which
  seconds usually are), but 120 fits into `int8_t`'s value space used by
  `avr-libc`'s `struct tm`.
`msp430-libc` does not provide the `mktime()` function used by those
tests.
Range errors were fixed and MSP-430 blacklisted using
`FEATURES_BLACKLIST` for the `tests-rtc` test suite.
@miri64 miri64 added Area: tests Area: tests and testing framework Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 14, 2019
@miri64 miri64 added this to the Release 2020.01 milestone Oct 14, 2019
@miri64 miri64 requested review from kaspar030 and maribu October 14, 2019 14:51
@miri64
Copy link
Member Author

miri64 commented Oct 14, 2019

@cladmi You introduced this blacklisting originally in 1ec4e45 (piggy-backed in #12040). Are you okay with this change?

@benpicco benpicco added the CI: run tests If set, CI server will run tests on hardware for the labeled PR label Oct 14, 2019
@smlng
Copy link
Member

smlng commented Oct 15, 2019

blacklist looks okay and works as expected, however now (in theory) Arduino-uno is supported for some tests, e.g. tests-rtc but that fails with:

2019-10-15 09:17:44,670 # rtc_tests.test_rtc_year (tests/unittests/tests-rtc/tests-rtc.c 25) exp 0 was 1

which might not be an issue exactly with the changes here, but for starters you might want to add arch_avr8 to the feature blacklist so we just remove the board blacklist first and replace with feature blacklist. Then try to fix the numbers of in rtc and pkt tests to support non 32bit platforms.

Maybe this should be split in 2 PRs, first replace boards with feature blacklist and afterwards the numerical stuff?!

@miri64
Copy link
Member Author

miri64 commented Oct 15, 2019

Maybe this should be split in 2 PRs, first replace boards with feature blacklist and afterwards the numerical stuff?!

Mh, it's about perspective. My approach was: fix the compile errors first, then remove the blacklisting. This you can only do in one PR. But can split.

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Oct 15, 2019
@smlng
Copy link
Member

smlng commented Oct 15, 2019

Maybe this should be split in 2 PRs, first replace boards with feature blacklist and afterwards the numerical stuff?!

Mh, it's about perspective. My approach was: fix the compile errors first, then remove the blacklisting. This you can only do in one PR. But can split.

I agree. But you actually do 3 things:

  • fix compile errors
  • change the way we blacklist
  • and remove blacklisting

I suggest to change the blacklist from boards for feature based in a separate PR, as we do for examples now. And fix compile errors and remove blacklist here (based on the other PR).

@miri64
Copy link
Member Author

miri64 commented Oct 15, 2019

I don't think we can fix the failing test btw, as this loop is causing an integer overflow... I'll make an alternative PR and remove the "fixes" to tests-rtc. I think we rather should think about either about changing struct tm for atmega or fixing the tests to accommodate for the different type there (maybe guard the overflowing tests with #ifdef MODULE_ARCH_AVR8.

@maribu
Copy link
Member

maribu commented Oct 15, 2019

e.g. tests-rtc but that fails with:

2019-10-15 09:17:44,670 # rtc_tests.test_rtc_year (tests/unittests/tests-rtc/tests-rtc.c 25) exp 0 was 1

Same on Arduino Mega2560

@miri64
Copy link
Member Author

miri64 commented Oct 15, 2019

e.g. tests-rtc but that fails with:

2019-10-15 09:17:44,670 # rtc_tests.test_rtc_year (tests/unittests/tests-rtc/tests-rtc.c 25) exp 0 was 1

Same on Arduino Mega2560

Since they use the same struct tm type, this is least surprising.

@maribu
Copy link
Member

maribu commented Oct 15, 2019

I don't think we can fix the failing test btw, as this loop is causing an integer overflow...

for (int i = 0; i <= 2*366; ++i) {

This loop? int is 16 bit on AVR, so it is not i this is overflowing. Or did you mean another variable that overflows?

@miri64
Copy link
Member Author

miri64 commented Oct 15, 2019

This loop? int is 16 bit on AVR, so it is not i this is overflowing. Or did you mean another variable that overflows?

Yeah, The int there is not the problem, but all the increments and decrements to tm_* within that loop.

@miri64
Copy link
Member Author

miri64 commented Oct 15, 2019

See #12455

@miri64 miri64 closed this Oct 15, 2019
@miri64 miri64 deleted the tests/enh/unittests-non-32-bit branch October 15, 2019 07:52
@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants