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

[Happy] Enable happy test in pre-submit checks #2688

Merged
merged 68 commits into from
Oct 13, 2020

Conversation

erjiaqing
Copy link
Contributor

@erjiaqing erjiaqing commented Sep 16, 2020

Problem

  • Currently, Happy test framework was added to CHIP, but no tests are enabled.

Summary of Changes

  • Follow-ups after [happy] Initial commit for adding Happy test #2319 is merged
  • Wrapper for running happy tests in gn/ninja
  • happy_test template
  • Enables them in pre-submit checks
  • Run all unit tests in gn_test
  • Happy tests can be invoked by
    scripts/build/gn_gen.sh
    RUN_HAPPY_TESTS=1 scripts/tests/gn_tests.sh
    
  • Move all unit tests to seperate github actions (unit_test).
  • Logs are uploads to artifacts in github actions

fixes #2591

@mspang
Copy link
Contributor

mspang commented Oct 12, 2020

Marking request changes for removal of root access requirement and use of sudo when running the tests via ninja. Please ping me if next steps are not clear.

PR Updated, found sudo is not available when using user namespace, added a face_sudo.sh as a workaround.

Why is this required?

From happy source:

    def getRunAsRootPrefixList(self):
        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"]]
        elif os.getuid() == 0:
            # We are already root
            return []
        else:
            return ["sudo"]

The getuid() check should work out, right?

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, just a few more fixes..

src/inet/tests/BUILD.gn Outdated Show resolved Hide resolved
src/inet/tests/BUILD.gn Outdated Show resolved Hide resolved
src/inet/tests/Makefile.am Outdated Show resolved Hide resolved
@erjiaqing
Copy link
Contributor Author

Marking request changes for removal of root access requirement and use of sudo when running the tests via ninja. Please ping me if next steps are not clear.

PR Updated, found sudo is not available when using user namespace, added a face_sudo.sh as a workaround.

Why is this required?

From happy source:

    def getRunAsRootPrefixList(self):
        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"]]
        elif os.getuid() == 0:
            # We are already root
            return []
        else:
            return ["sudo"]

The getuid() check should work out, right?

In happy:

    def getRunAsUserPrefixList(self, username=None):
        if username is None:
            username = getpass.getuser()

        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"], "-u", username]
        else:
            return ["sudo", "-u", username]

sudo is required here.

src/BUILD.gn Outdated Show resolved Hide resolved
@mspang
Copy link
Contributor

mspang commented Oct 12, 2020

Marking request changes for removal of root access requirement and use of sudo when running the tests via ninja. Please ping me if next steps are not clear.

PR Updated, found sudo is not available when using user namespace, added a face_sudo.sh as a workaround.

Why is this required?
From happy source:

    def getRunAsRootPrefixList(self):
        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"]]
        elif os.getuid() == 0:
            # We are already root
            return []
        else:
            return ["sudo"]

The getuid() check should work out, right?

In happy:

    def getRunAsUserPrefixList(self, username=None):
        if username is None:
            username = getpass.getuser()

        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"], "-u", username]
        else:
            return ["sudo", "-u", username]

sudo is required here.

Can you file a bug against happy and put a TODO linking to it in CHIP ?

@mspang
Copy link
Contributor

mspang commented Oct 12, 2020

Marking request changes for removal of root access requirement and use of sudo when running the tests via ninja. Please ping me if next steps are not clear.

PR Updated, found sudo is not available when using user namespace, added a face_sudo.sh as a workaround.

Why is this required?
From happy source:

    def getRunAsRootPrefixList(self):
        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"]]
        elif os.getuid() == 0:
            # We are already root
            return []
        else:
            return ["sudo"]

The getuid() check should work out, right?

In happy:

    def getRunAsUserPrefixList(self, username=None):
        if username is None:
            username = getpass.getuser()

        if "SUDO" in list(os.environ.keys()):
            return [os.environ["SUDO"], "-u", username]
        else:
            return ["sudo", "-u", username]

sudo is required here.

Can you file a bug against happy and put a TODO linking to it in CHIP ?

Actually, I still don't think this is required. Looks like that function will not be run if we're already root.

@mspang
Copy link
Contributor

mspang commented Oct 12, 2020

  • Happy tests can be invoked by RUN_HAPPY_TESTS=1 scripts/tests/gn_tests.sh

This doesn't quite work, can you add some documentation before we add these to presubmit?

+ ninja -v -C scripts/tests/../../out/ -k 0 check
ninja: Entering directory `scripts/tests/../../out/'
ninja: error: loading 'build.ninja': No such file or directory

(These GitHub action scripts don't work well locally)

@erjiaqing
Copy link
Contributor Author

  • Happy tests can be invoked by RUN_HAPPY_TESTS=1 scripts/tests/gn_tests.sh

This doesn't quite work, can you add some documentation before we add these to presubmit?

+ ninja -v -C scripts/tests/../../out/ -k 0 check
ninja: Entering directory `scripts/tests/../../out/'
ninja: error: loading 'build.ninja': No such file or directory

(These GitHub action scripts don't work well locally)

Added one more step for gn gen

@erjiaqing erjiaqing requested a review from mspang October 13, 2020 06:55
@mspang
Copy link
Contributor

mspang commented Oct 13, 2020

The flow works reasonably well now and without extraordinary privileges, thank you.

I think we should followup with some further workflow fixes, e.g. it should work in parallel with the rest of the test suite and it should not print output to the console if it passes.

@mspang mspang merged commit 30123da into project-chip:master Oct 13, 2020
@mspang
Copy link
Contributor

mspang commented Oct 13, 2020

I think we should followup with some further workflow fixes, e.g. it should work in parallel with the rest of the test suite and it should not print output to the console if it passes.

#3214

andy31415 pushed a commit that referenced this pull request Oct 16, 2020
* Follow ups of happy tests

* Add redirect option

* Capture test output

* Add check

* Fix document

* Restyled by prettier-markdown

* Remove unused import

Co-authored-by: Restyled.io <commits@restyled.io>
@erjiaqing erjiaqing deleted the enable-happy-test branch January 29, 2021 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HAPPY] Add parallel test engine for Happy
8 participants