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

Add ability to override the log alias directory. #584

Merged

Conversation

winterfroststrom
Copy link
Contributor

@winterfroststrom winterfroststrom commented Jun 22, 2019

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)


mobly/logger.py, line 26 at r1 (raw file):

from mobly import utils

LATEST_LOG_ALIAS = 'latest'

When is the appropriate time to call this from a user's perspective?
The log dir is immediately created upon Mobly running right?

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)


mobly/logger.py, line 26 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

When is the appropriate time to call this from a user's perspective?
The log dir is immediately created upon Mobly running right?

Before setup_logger from test_runner is called, which basically means in whatever calls test_runner such as the main function.

@winterfroststrom winterfroststrom force-pushed the optional_latest_log_alias branch from bd4f037 to 8b41098 Compare June 24, 2019 20:09
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)


mobly/logger.py, line 26 at r1 (raw file):

Previously, winterfroststrom wrote…

Before setup_logger from test_runner is called, which basically means in whatever calls test_runner such as the main function.

Is there's a way to surface this usage in the official API doc.
Because I doubt many people would even be aware of this "feature" if we don't explicitly surface it.

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)


mobly/logger.py, line 26 at r1 (raw file):

Previously, xpconanfan (Ang Li) wrote…

Is there's a way to surface this usage in the official API doc.
Because I doubt many people would even be aware of this "feature" if we don't explicitly surface it.

added something to the docstring of create_latest_log_alias

@winterfroststrom winterfroststrom force-pushed the optional_latest_log_alias branch from a96e2a8 to 766f819 Compare July 9, 2019 01:05
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, 3 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)


mobly/logger.py, line 195 at r2 (raw file):

    """Creates a symlink to the latest test run logs.

    The latest log alias directory uses the value of `logger.LATEST_LOG_ALIAS`.

how about "Path to the most recent set of logs uses"


mobly/logger.py, line 204 at r2 (raw file):

        actual_path: The source directory where the latest test run's logs are.
    """
    if LATEST_LOG_ALIAS:

actually, should we just make this a param of setup_logger?
would be much easier to document and use?

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, 3 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)


mobly/logger.py, line 204 at r2 (raw file):

Previously, xpconanfan (Ang Li) wrote…

actually, should we just make this a param of setup_logger?
would be much easier to document and use?

Well, I was originally thinking of a use case where test_runner isn't directly accessible (from an end user perspective since it's wrapped something else), but I guess it technically is in that use case.
There's a couple different things that this does here though:

  1. allow the alias dir to not be created
  2. allow getting the name of the alias dir
  3. allow changing the alias dir name to something else

I don't see any easy way for (2) without additional changes to the test runner config? Unless we want to assume the end user is using test runner in a way where they can handle that themselves?

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, 3 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)


mobly/logger.py, line 204 at r2 (raw file):

Previously, winterfroststrom wrote…

Well, I was originally thinking of a use case where test_runner isn't directly accessible (from an end user perspective since it's wrapped something else), but I guess it technically is in that use case.
There's a couple different things that this does here though:

  1. allow the alias dir to not be created
  2. allow getting the name of the alias dir
  3. allow changing the alias dir name to something else

I don't see any easy way for (2) without additional changes to the test runner config? Unless we want to assume the end user is using test runner in a way where they can handle that themselves?

why is (2) important?
if the user is setting it via params, they should know what they set it to?...

@winterfroststrom winterfroststrom force-pushed the optional_latest_log_alias branch from 766f819 to 20e21a0 Compare July 25, 2019 20:42
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 4 files reviewed, 3 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)


mobly/logger.py, line 204 at r2 (raw file):

Previously, xpconanfan (Ang Li) wrote…

why is (2) important?
if the user is setting it via params, they should know what they set it to?...

changed

@winterfroststrom winterfroststrom force-pushed the optional_latest_log_alias branch from c95a8c9 to ac3af99 Compare July 31, 2019 01:49
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 4 files reviewed, 2 unresolved discussions (waiting on @winterfroststrom)


mobly/logger.py, line 213 at r3 (raw file):

            are requested.
        alias: The name of the alias to use for the latest log directory. If a
          falsy value is provided, then the alias directory will not be created.

explain when does it make sense to not create alias?


mobly/logger.py, line 218 at r3 (raw file):

    _setup_test_logger(log_path, prefix)
    logging.info('Test output folder: "%s"', log_path)
    create_latest_log_alias(log_path, alias=alias)

how about moving the if alias check here and not call the create alias method?
also remove the default value of alias in create_latest_log_alias

It is a bit odd to allow calling something called "create_latest_log_alias" and not have it do anything...

@xpconanfan xpconanfan added this to the Mobly Release 1.9 milestone Aug 1, 2019
@winterfroststrom winterfroststrom force-pushed the optional_latest_log_alias branch from 6f1a0bc to 543c673 Compare August 1, 2019 20:04
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 4 files reviewed, 2 unresolved discussions (waiting on @winterfroststrom and @xpconanfan)


mobly/logger.py, line 213 at r3 (raw file):

Previously, xpconanfan (Ang Li) wrote…

explain when does it make sense to not create alias?

added blurb


mobly/logger.py, line 218 at r3 (raw file):

Previously, xpconanfan (Ang Li) wrote…

how about moving the if alias check here and not call the create alias method?
also remove the default value of alias in create_latest_log_alias

It is a bit odd to allow calling something called "create_latest_log_alias" and not have it do anything...

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.

:lgtm:

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


mobly/logger.py, line 194 at r4 (raw file):

    Args:
        actual_path: The source directory where the latest test run's logs are.

the args sections in these two functions are missing the arg type.
Can you add the types...

@winterfroststrom winterfroststrom merged commit 7026d69 into google:master Aug 1, 2019
@winterfroststrom winterfroststrom deleted the optional_latest_log_alias branch September 13, 2019 02:24
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.

2 participants