8000 [collectd 6] Use real collectd metric functions in gpu_sysman plugin tests by eero-t · Pull Request #4223 · collectd/collectd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[collectd 6] Use real collectd metric functions in gpu_sysman plugin tests #4223

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 20, 2024
Merged

[collectd 6] Use real collectd metric functions in gpu_sysman plugin tests #4223

merged 1 commit into from
Feb 20, 2024

Conversation

eero-t
Copy link
Contributor
@eero-t eero-t commented Jan 3, 2024 •
edited by octo
Loading

ChangeLog: gpu_sysman plugin: Use real collectd metric functions unit tests.

There have been lot of changes in metric handling code, so it's better to use real metric functions rather than mocked ones.

This allows removing the mock functions, but makes it much easier for unrelated changes to break gpu_sysman_test.c compilation (e.g .utf-8 support already broke earlier version of this).

@eero-t eero-t requested review from a team as code owners January 3, 2024 19:27
@collectd-bot collectd-bot added this to the 6.0 milestone Jan 3, 2024
@eero-t
Copy link
Contributor Author
eero-t commented Jan 3, 2024 •
edited
Loading

@octo CI fails to unrelated error on "debian10" (although "debian11" passes):

  Submodule 'gnulib' (git://git.savannah.gnu.org/gnulib.git) registered for path 'gnulib'
  Submodule 'opentelemetry-proto' (https://github.com/open-telemetry/opentelemetry-proto/) registered for path 'opentelemetry-proto'
  Cloning into '/__w/collectd/collectd/gnulib'...
  Cloning into '/__w/collectd/collectd/opentelemetry-proto'...
  Error: fatal: read error: Connection reset by peer
  Fetched in submodule path 'gnulib', but it did not contain 2f8140bc8ce5501e31dcc665b42b5df64f84c20c. Direct fetching of that commit failed.
  Error: The process '/usr/bin/git' failed with exit code 1

@eero-t eero-t mentioned this pull request Jan 3, 2024
Merged
@eero-t
Copy link
Contributor Author
eero-t commented Jan 3, 2024

@octo CI fails to unrelated error on "debian10" (although "debian11" passes):

Oddly, everything passed in #4177 that was rebased on top of this. Maybe that error was just transitory network error.

octo
octo approved these changes Jan 3, 2024
View reviewed changes
@octo
Copy link
Member
octo commented Jan 3, 2024

@octo CI fails to unrelated error on "debian10" (although "debian11" passes):

Oddly, everything passed in #4177 that was rebased on top of this. Maybe that error was just transitory network error.

git.savannah.gnu.org had transient errors all day. The "Actions" tab has a "re-run failed workflows" button that is quite useful in that situation.

octo
octo approved these changes Jan 10, 2024
View reviewed changes
Copy link
Member
@octo octo left a comment

Choose a reason for hiding this comment

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

LGTM

8000
@eero-t eero-t force-pushed the sysman-test-metrics branch from 31c67d1 to 8529469 Compare January 15, 2024 16:46
@eero-t eero-t added the Maintenance A pull request without immediate user-observable impact label Jan 15, 2024
@eero-t eero-t force-pushed the sysman-test-metrics branch from 212ba71 to 3a73d74 Compare January 15, 2024 17:10
@eero-t
Copy link
Contributor Author
eero-t commented Jan 15, 2024

Rebased to to latest collectd-6.0, fixed new/remaining GCC warnings in collectd core code, and ordered commits so that every one of them compiles with stricter compiler options.

(Sorry for the extra pushes, annoyingly my local and CI clang-format disagree on include order.)

@octo I had to downgrade one warning in my own tests due to this:

$ gcc -v
gcc version 13.2.1 20231011 (Red Hat 13.2.1-4) (GCC)

$ gcc -O2 -I src -I src/daemon -DFP_LAYOUT_NEED_NOTHING=1 -Werror -Wstrict-overflow=4 src/utils/common/common.c 
src/utils/common/common.c: In function ‘walk_directory’:
src/utils/common/common.c:1146:5: error: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Werror=strict-overflow]
 1146 | int walk_directory(const char *dir, dirwalk_callback_f callback,
      |     ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors

-Wstrict-overflow takes values from 1-5, and any value above 2 gives above warning when GCC optimization level is >1.

I do not see what GCC could be complaining about common.c though.

@eero-t eero-t force-pushed the sysman-test-metrics branch 3 times, most recently from 6b55c5b to b8483f3 Compare January 17, 2024 19:52
@eero-t
Copy link
Contributor Author
eero-t commented Jan 17, 2024

I hadn't understood how collectd code handles system includes (= nearly all code relies on them being included from collectd.h, but that happens only when HAVE_CONFIG_H is defined, i.e. configure has generated config.h etc).

=> Reverted my include addition commits, cleaned up those from test-code, and documented how tests should be run manually.

octo
octo approved these changes Jan 22, 2024
View reviewed changes
@eero-t eero-t force-pushed the sysman-test-metrics branch from b8483f3 to 9459fc0 Compare January 29, 2024 17:59
@eero-t
Copy link
Contributor Author
eero-t commented Jan 29, 2024

Current version is failing because config.h apparently gets configured with HAVE_CAPABILITY in some CI targets, and therefore:

/__w/collectd/collectd/src/utils/common/common.c:1485: undefined reference to `cap_get_bound'
/usr/bin/ld: /__w/collectd/collectd/src/utils/common/common.c:1488: undefined reference to `cap_get_proc'
/usr/bin/ld: /__w/collectd/collectd/src/utils/common/common.c:1493: undefined reference to `cap_get_flag'
/usr/bin/ld: /__w/collectd/collectd/src/utils/common/common.c:1498: undefined reference to `cap_free'
/usr/bin/ld: /__w/collectd/collectd/src/utils/common/common.c:1495: undefined reference to `cap_free'

=> Trying to include real *.c files directly is too much whack-a-mole game.

I guess I'll next need to try how badly testing breaks if tester is instead linked with libplugin_mock.la, and calls Sysman plugin's plugin-API functions directly.

Looking at Makefile.am, there does not seem to be any archive that I could use to link all relevant real functions, as closest to that seemed to be what collectd daemon itself links.

@octo
Copy link
Member
octo commented Jan 29, 2024

Looking at Makefile.am, there does not seem to be any archive that I could use to link all relevant real functions, as closest to that seemed to be what collectd daemon itself links.

That is was libplugin_mock.la needs to become IMHO – a drop-in replacement for the daemon to test plugins. That's for example why I recently threw out the mocked/faked "cache" implementation and linked with the real one instead. By linking with the "real" implementation tests can now cover plugins that require the cache to function.

@eero-t eero-t force-pushed the sysman-test-metrics branch 3 times, most recently from 8b1412b to 58f81de Compare February 15, 2024 14:01
@eero-t
Copy link
Contributor Author
eero-t commented Feb 15, 2024 •
edited
Loading

@octo This works OK again, and your feedback should be handled.

As to my removal of (apparently redundant) -lm lines from Makefile.am, should I split that to separate PR?

EDIT: if you'll prefer, I can also squash the other commits to a single one before merging.

@eero-t eero-t force-pushed the sysman-test-metrics branch 2 times, most recently from 584ca69 to 8a36918 Compare February 15, 2024 15:21
@eero-t eero-t mentioned this pull request Feb 15, 2024
Closed
@eero-t
gpu_sysman_test.c: use real collectd functions in tests
896eaa3 
Drop most mocked functions and use real ones by linking libcommon.la.
This required mocking few other functions that it refers (but does not
link).

Support for a build including relevant C-files directly, e.g. for
whole program analysis, is added behind BUILD_STANDALONE define.

(It's also a fallback in case libcommon.la later changes to include
functionality that tester must override to be able to check correct
plugin functionality.)

Signed-off-by: Eero Tamminen <eero.t.tamminen@intel.com>
@eero-t eero-t force-pushed the sysman-test-metrics branch from 8a36918 to 896eaa3 Compare February 20, 2024 13:27
@eero-t
Copy link
Contributor Author
eero-t commented Feb 20, 2024

Dropped the extra Makefile.am change, rebased to latest & squashed commits to a single one. Will merge this later today (based on the earlier approval, unless there are comments/objections/CI-check fails).

@eero-t eero-t merged commit f73e50a into collectd:collectd-6.0 Feb 20, 2024
@eero-t eero-t deleted the sysman-test-metrics branch February 20, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@octo octo octo approved these changes

Assignees
No one assigned
Labels
Maintenance A pull request without immediate user-observable impact
Projects
None yet
Milestone
6.0
Development

Successfully merging this pull request may close these issues.
3 participants
@eero-t @octo @collectd-bot
0