-
Notifications
You must be signed in to change notification settings - Fork 65
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
Misc test tweaks #82
Misc test tweaks #82
Conversation
The tests need to be run with root/sudo privs and it would be best to do so the minimal amount of time, so we should not be compiling the test with root privileges. This means we can have the test binary around and thus it should be ignored.
t.Logf is quiet unless a test fails or -v is used. It also keeps the log messages grouped with the test names nicely.
It isn't a helper.
This way we can avoid a level of indentation.
Sucks having to scroll through helpers to get to the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review zfs_test.go very closely. I'm assuming that the tests are generally still the same...
ok(t, err) | ||
equals(t, zfs.DatasetFilesystem, ds.Type) | ||
equals(t, "", ds.Origin) | ||
if runtime.GOOS != "solaris" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if/why this is needed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't review zfs_test.go very closely. I'm assuming that the tests are generally still the same...
Yep tests still the same, just dedented one tab for the most part (and a simple string test fail update).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if/why this is needed...
No clue, I'd like to try setting up a vagrant vm with omnios and smartos for some tests. iirc this may have come in from when someone from oracle PR'd some solaris stuff. But I don't have a way to test that so omni/smart os are going to be my solaris boxes for features and what not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OmniOS VM in GitHub actions:
https://github.com/nshalman/fsnotify/blob/fen-v2/.github/workflows/slow-tests.yml
Just some minor reworks for readability.