-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix or disable failing tests #5614
Conversation
Fix ebpfTests.sysEbpf_null_attr, ebpfTests.sysEbpf_create_map, ebpfMapTests.int_key_int_value, ebpfMapTests.int_key_struct_value by running Docker as privileged on Azure Pipelines. Docker is used only to get a new distribution running, it's not used for any security purpose, so there's no point in limiting it.
Do not checkout with carriage returns on Windows, otherwise it would mess up with code that expect unix style files.
Fix UsersTest.test_sanity on Windows. uid and gid were returned as int (while they normally are unsigned int) and converted to signed integers in the table row. This is wrong because beyond uid and gid not being ints, they are taken from the RID part of the SID which in some cases, like for a Service SID, it can have a value higher than then maximum value of an int, so in the end the number shown in table is negative. Now they are returned as uint32_t and converted as BIGINTs for the table that uses them. Fix other functions return values and conversions depending on the meaning of the value. On Windows stick to its specific types where possible. Convert CRLF to LF on some of the files modified.
The table it tests has several issues, it doesn't properly check for errors in several places and returns unexpected values in the columns. Moreover the code could also be improved to make testing possible without passing through SQL queries, by separating the part that uses WMI queries from the conversion to row results.
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.
Looks reasonable to me. (I leave CI and tests to verify legitimacy)
Thanks for the review! Sorry I noticed that I mixed variable naming styles; when the PR will be ready to be merged I'll squash that with its respective commit (b3f7ab3) |
The code was failing on Windows because the '\Windows\%' pattern is relative and presumes that the Windows folder is on the same drive (C:\) as the test process. This might not be true, so we find where precisely is the Windows directory and use the full path to it as a pattern. The failing test checks that were testing the equal and LIKE operator have been moved to a new test, test_table_constraints, since they are not related to joins.
I've squashed now, I remembered that it would make reviews stale again if I did it later. |
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.
Seems fine.
@@ -26,6 +28,11 @@ class InterfaceDetailsTest : public testing::Test { | |||
}; | |||
|
|||
TEST_F(InterfaceDetailsTest, test_sanity) { | |||
#ifdef OSQUERY_WINDOWS | |||
LOG(INFO) << "Test failing on Windows, temporarily disabled"; |
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'd wish we had a consistent pattern for this, something to more easily look for. But this seems fine
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.
We can eventually have a function in some existing test helper code that checks for a given list of platforms and prints a standard message.
Obviously there are a couple of other ways to disable tests, which though either disable entirely a single test case or a test suite.
For googletest this is prepending DISABLED_ to the test case name (for instance TEST_F(InterfaceDetailsTest, DISABLED_test_sanity
) or test suite name (for instance class DISABLED_InterfaceDetailsTest : public testing::Test {
)
@Smjert I've been assuming you can [technically] merge this with my approval, and will do so when you choose? Or are you waiting on someone? |
I'll merge it, I was mulling over unifying some other tests changes to this but I already made different PRs. |
Start working toward getting the Azure Pipelines CI all green.
#5608