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

Unittest redesign #58

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

ledvinap
Copy link

@ledvinap ledvinap commented May 2, 2019

Build printf using C
Convert most tests in test_suite to use macro
Fix catch2

New tests use CHECK now, but it may be reverted easily back to REQUIRE.

@codecov-io
Copy link

codecov-io commented May 2, 2019

Codecov Report

Merging #58 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #58   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines         359    355    -4     
=====================================
- Hits          359    355    -4
Impacted Files Coverage Δ
printf.c 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21eea6c...ccf6149. Read the comment docs.

avoid collision with libinsanity macro naming
Some tests got duplicated, one version may removed eventually.
It is probably good idea to keep changes to libinsanity as small as
possible to allow future merging

Some tests did discover printf bugs/inconsistencies and are failing intentionally
move code into separate scope
Considerably reduces compilation time. gcc seems to have problems with
multiple tests reusing variables
@ledvinap
Copy link
Author

Hello.
Is there anything I can do ?

The coverage report is strange - missing line is always-false branch dependent on type size (sizeof(uintptr_t) == sizeof(long long);), it shouldn't be covered even before this change, but c++ is probably optimizing a bit even with -O0

idx = _ntoa_long(out, buffer, idx, maxlen, (unsigned long)((uintptr_t)va_arg(va, void*)), false, 16U, precision, width, flags);
#if defined(PRINTF_SUPPORT_LONG_LONG)
}
#if defined(PRINTF_SUPPORT_LONG_LONG) && (LONG_MAX < UINTPTR_MAX)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test should be if defined(PRINTF_SUPPORT_LONG_LONG) && (ULONG_MAX < UINTPTR_MAX). The current test selects the long long code even for 32-bit pointers on targets with 32-bit longs as LONG_MAX < ULONG_MAX is always true. Writing this comparison as (UINTPTR_MAX > ULONG_MAX) might be more readable.

@eyalroz
Copy link

eyalroz commented Jun 28, 2021

@ledvinap : So, some of these changes aren't strictly about the unit tests, and are covered by other PRs. Also, if you check out my fork, you'll notice I've merged some of those proposed changes, as this repo has not seen much traction for a while. If you're interested - I would appreciate PR against that fork.

KarlK90 pushed a commit to qmk/printf that referenced this pull request Jul 7, 2022
…of the source files into a library-specific subdirectory, as per mpaland#58, there is no longer the risk of clashing with glibc's `printf.h`, so the test programs can use `<foo.h>`-style inclusion rather than `"../path/to/foo.h"`-style.
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.

None yet

4 participants