Page MenuHomeFreeBSD

libc: add some tests for localtime()
Needs ReviewPublic

Authored by kevans on Dec 16 2021, 2:20 AM.
Tags
None
Referenced Files
F102558656: D33493.id100116.diff
Thu, Nov 14, 1:28 AM
Unknown Object (File)
Thu, Oct 31, 12:00 PM
Unknown Object (File)
Thu, Oct 31, 11:20 AM
Unknown Object (File)
Sun, Oct 27, 8:23 AM
Unknown Object (File)
Fri, Oct 18, 8:37 AM
Unknown Object (File)
Oct 5 2024, 7:36 AM
Unknown Object (File)
Oct 2 2024, 3:03 PM
Unknown Object (File)
Oct 2 2024, 1:44 AM
Subscribers

Details

Reviewers
allanjude
trasz
markj
Group Reviewers
tests
Summary

Test gmt offset information obtained from localtime(), as well as
ensuring that timezone changes either are or aren't detected depending
on how the test was built.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 43402
Build 40290: arc lint + arc unit

Event Timeline

Add a couple more folks to hopefully get a sanity check here; setting up a chroot in the testing directory and copying parts of the host in seems a little unconventional, but hopefully it's OK enough given the circumstances. The test is intended to skip gracefully if the host doesn't have /usr/share/zoneinfo or a specific zone that's being used.

I understand and appreciate what you're striving for, but a major word of caution: tests that involve this degree of complexity have lower determinism, reducing the level of trust in a given test over the long run.

Warning: these comments are based on dated knowledge -- I've had my head in the sand with $work and haven't been keeping up with the state of the art with FreeBSD.

This testcases might not work with all secure levels/security implementations and implicitly builds in a dependency on being run as root (which hasn't been called out in the requirements for the testcases). This also unfortunately violates the ATF sandbox potentially and might not play well with capability mode (when I last looked at it) as it requires file descriptors to be inherited for writing out logs (should work, but.. the way it's done was fragile).

I feel like ATF is the wrong thing for doing these kinds of tests. I really wish we had a better way of invoking/orchestrating integration tests, not just for FreeBSD, but also other consumers of FreeBSD.

In D33493#759412, @ngie wrote:

I understand and appreciate what you're striving for, but a major word of caution: tests that involve this degree of complexity have lower determinism, reducing the level of trust in a given test over the long run.

Warning: these comments are based on dated knowledge -- I've had my head in the sand with $work and haven't been keeping up with the state of the art with FreeBSD.

This testcases might not work with all secure levels/security implementations and implicitly builds in a dependency on being run as root (which hasn't been called out in the requirements for the testcases). This also unfortunately violates the ATF sandbox potentially and might not play well with capability mode (when I last looked at it) as it requires file descriptors to be inherited for writing out logs (should work, but.. the way it's done was fragile).

Virtually all of our regression tests will not work in capability mode. Capsicum generally requires all sandboxed code to be aware of the sandbox since many common operations are not permitted.

The kernel securelevel doesn't restrict chroot (which is used by many daemons to restrict filesystem access), and I don't see how it'd otherwise be relevant here.

I feel like ATF is the wrong thing for doing these kinds of tests. I really wish we had a better way of invoking/orchestrating integration tests, not just for FreeBSD, but also other consumers of FreeBSD.

I'm not a fan of ATF, but I don't immediately see which of its assumptions are being violated by running tests in a chroot. By default test results are written to standard streams, and otherwise are written to a pre-opened file descriptor.

lib/libc/tests/time/localtime_test.c
2

Add an SPDX identifier?

83

The blank line here is optional, btw.

127

As ngie points out, the tests need to set required_user=root in the test metadata.

148

Check return values from setenv() and unsetenv()?

159

I'd check the return value of unlink().

219

I think it'd be good to have some high-level comment, perhaps at the beginning of the file, explaining the purpose of the tests.