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

Fix or disable failing tests #5614

Merged
merged 5 commits into from
Jul 9, 2019
Merged

Conversation

Smjert
Copy link
Member

@Smjert Smjert commented Jun 29, 2019

Start working toward getting the Azure Pipelines CI all green.

#5608

@Smjert Smjert added the test label Jun 29, 2019
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.
@Smjert Smjert marked this pull request as ready for review July 2, 2019 23:25
directionless
directionless previously approved these changes Jul 4, 2019
Copy link
Member

@directionless directionless left a 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)

@Smjert
Copy link
Member Author

Smjert commented Jul 4, 2019

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.
@Smjert
Copy link
Member Author

Smjert commented Jul 4, 2019

I've squashed now, I remembered that it would make reviews stale again if I did it later.

Copy link
Member

@directionless directionless left a 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";
Copy link
Member

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

Copy link
Member Author

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 {)

@directionless
Copy link
Member

@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?

@Smjert
Copy link
Member Author

Smjert commented Jul 9, 2019

@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.

@Smjert Smjert merged commit 2f681e7 into osquery:master Jul 9, 2019
@Smjert Smjert deleted the stefano/fix/tests branch October 23, 2019 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants