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 suite_runner with logger. #610

Merged

Conversation

winterfroststrom
Copy link
Contributor

@winterfroststrom winterfroststrom commented Jul 31, 2019

In #606, suite_runner was not changed and had not tests and I didn't notice it, so this fixes that


This change is Reviewable

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

        tmp_file_path = os.path.join(self.tmp_dir, 'config.yml')
        with io.open(tmp_file_path, 'w', encoding='utf-8') as f:
            f.write(u'TestBeds:\n')

can we not write one big string instead of separate write statements?

Copy link
Contributor Author

@winterfroststrom winterfroststrom left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

can we not write one big string instead of separate write statements?

Done.

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, winterfroststrom wrote…

Done.

If you want to inline them, you can do
("blah:"
" secondary blah:"
" more indent blah")

Copy link
Contributor Author

@winterfroststrom winterfroststrom left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

If you want to inline them, you can do
("blah:"
" secondary blah:"
" more indent blah")

I mean I could also do that with a heredoc, is there a preference here?

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, winterfroststrom wrote…

I mean I could also do that with a heredoc, is there a preference here?

slight preference for not having macros that can't be indented...

Copy link
Contributor Author

@winterfroststrom winterfroststrom left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

slight preference for not having macros that can't be indented...

I'm not sure what you mean by indented?

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, winterfroststrom wrote…

I'm not sure what you mean by indented?

Like you can't indent the actual string in code, and this looks weird:

        f.write(u"""

TestBeds:
# A test bed where adb will find Android devices.
- Name: SampleTestBed
Controllers:
MagicDevice: '*'
""")

Copy link
Contributor Author

@winterfroststrom winterfroststrom left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)


tests/mobly/suite_runner_test.py, line 90 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Like you can't indent the actual string in code, and this looks weird:

        f.write(u"""

TestBeds:
# A test bed where adb will find Android devices.
- Name: SampleTestBed
Controllers:
MagicDevice: '*'
""")

I'm not sure why you want this to be indented since that seems less readable than just a macro string to me, but okay

Copy link
Collaborator

@xpconanfan xpconanfan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @winterfroststrom and @xpconanfan)

@winterfroststrom winterfroststrom merged commit a18569f into google:master Jul 31, 2019
@winterfroststrom winterfroststrom deleted the add_missing_logger_contxt branch September 13, 2019 02:24
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