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.

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

Initialize users and groups services on all tests that need them #7620

Merged
merged 1 commit into from
Jul 3, 2022
Merged

Initialize users and groups services on all tests that need them #7620

merged 1 commit into from
Jul 3, 2022

Conversation

Smjert
Copy link
Member

@Smjert Smjert commented Jun 7, 2022 •
edited

Some of the tests making queries on tables that were
indirectly making use of the users and groups cached information
were sometimes failing because the needed services were not started.
This starts the appropriate services and makes sure to reset
the futures needed to access the cache, so that all the tests
that do not initialize the services on their own will fail,
instead of relying on a previous initialization.

@Smjert Smjert added bug flaky test A unit test that sometimes or on some systems does not return success, when it should labels Jun 7, 2022
@Smjert Smjert requested review from a team as code owners June 7, 2022 12:44
@Smjert Smjert added Windows and removed Windows labels Jun 7, 2022
@Smjert Smjert force-pushed the stefano/test/fix-user-groups-init branch from df27e98 to 43afb3c Compare June 7, 2022 12:48
@Smjert Smjert mentioned this pull request Jun 7, 2022
Merged
@Smjert Smjert force-pushed the stefano/test/fix-user-groups-init branch from 43afb3c to a7d8569 Compare June 7, 2022 13:40
directionless
directionless previously approved these changes Jun 11, 2022
View reviewed changes
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 as-is, though there's enough duplication it feels like it should be in a helper.

@mike-myers-tob
Copy link
Member

Seems fine as-is, though there's enough duplication it feels like it should be in a helper.

Agreed, shouldn't we move the definitions of SetUpTestSuite() and TearDownTestSuite() to /osquery/tests/test_util.cpp ?

@Smjert
Initialize users and groups services on all tests that need them
c07484b 
Some of the tests making queries on tables that were
indirectly making use of the users and groups cached information
were sometimes failing because the needed services were not started.
This starts the appropriate services and makes sure to reset
the futures needed to access the cache, so that all the tests
that do not initialize the services on their own will fail,
instead of relying on a previous initialization.
@Smjert Smjert force-pushed the stefano/test/fix-user-groups-init branch from a7d8569 to c07484b Compare June 14, 2022 14:45
@Smjert Smjert added this to the 5.4.0 milestone Jun 16, 2022
@Smjert Smjert added the ready for review Pull requests that are ready to be reviewed by a maintainer label Jun 27, 2022
@Smjert Smjert merged commit 215d9f6 into osquery:master Jul 3, 2022
@Smjert Smjert deleted the stefano/test/fix-user-groups-init branch July 3, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@directionless directionless directionless approved these changes

Assignees
No one assigned
Labels
bug flaky test A unit test that sometimes or on some systems does not return success, when it should ready for review Pull requests that are ready to be reviewed by a maintainer
Projects
None yet
Milestone
5.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Smjert @mike-myers-tob @directionless