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

Support for test listing, running and metrics via v3 #14

Merged
merged 25 commits into from
Oct 3, 2017

Conversation

diego-plan9
Copy link
Contributor

Additions to the cli for listing Tests, starting (with streaming) TestRuns, and listing Metrics, in tandem with the changes on the SDK at li-clutter-org/loadimpact-sdk-python#7.

Diego M. Rodriguez added 12 commits April 21, 2017 15:26
Add a "test" click command group containing a "list" command for
displaying an overview of the tests for the projects of the
organizations the user has access to (by default); or only for the tests
of particular projects via a project_id parameter.
Add a "metric" click command group containing a "list" command for
displaying an overview of the metrics for a test run; optinally
filterng by metric type.
Add metric streaming, via a configurable "--quiet" flag and multiple
"--metric" parameters.
@coveralls
Copy link

coveralls commented May 3, 2017

Coverage Status

Coverage increased (+0.2%) to 73.698% when pulling 8d052a4 on feature/apiv3-test-support into e68ca92 on develop.

@robingustafsson
Copy link
Collaborator

robingustafsson commented May 4, 2017

I'm testing this. Ran into an issue when listing my large list of tests :-)

Traceback (most recent call last):
  File "/usr/local/bin/loadimpact", line 9, in <module>
    load_entry_point('loadimpact-cli==1.0.6', 'console_scripts', 'loadimpact')()
  File "build/bdist.macosx-10.12-intel/egg/loadimpactcli/loadimpact_cli.py", line 41, in run_cli
  File "/Library/Python/2.7/site-packages/click/core.py", line 716, in __call__
    return self.main(*args, **kwargs)
  File "/Library/Python/2.7/site-packages/click/core.py", line 696, in main
    rv = self.invoke(ctx)
  File "/Library/Python/2.7/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Library/Python/2.7/site-packages/click/core.py", line 1060, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/Library/Python/2.7/site-packages/click/core.py", line 889, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/Library/Python/2.7/site-packages/click/core.py", line 534, in invoke
    return callback(*args, **kwargs)
  File "build/bdist.macosx-10.12-intel/egg/loadimpactcli/test_commands.py", line 57, in list_tests
UnicodeEncodeError: 'ascii' codec can't encode character u'\xe3' in position 59: ordinal not in range(128)

Diego M. Rodriguez added 2 commits May 5, 2017 11:18
Change the format for the "config summary" column (to "XX users YYs")
and colorize the "last run status" column on "test list" output.
@coveralls
Copy link

coveralls commented May 5, 2017

Coverage Status

Coverage increased (+0.9%) to 74.396% when pulling 5205ca0 on feature/apiv3-test-support into e68ca92 on develop.

Diego M. Rodriguez added 4 commits May 9, 2017 16:01
Introduce a `util.Metric` class for converting between different
representations (raw format expected by the api, user friendly name,
parameter name). Revise `metric list` command for displaying the name
of the metric when used as a parameter.
Revise `test run` syntax and output, displaying the metric values in
a single row, and accepting standard metrics and custom metrics.
Add unit tests for `util.Metric` (along with its usage) and for the
expected flow of calls during metric streaming, fixing a number of
issues in the process:
* fix unicode handling on test_commands
* add equality methods to metrics and metric types
* fix parameter handling for non-standard metrics, not appending the
load zone identifier automatically
* revise docstrings and minor tweaks
@coveralls
Copy link

coveralls commented May 10, 2017

Coverage Status

Coverage increased (+6.6%) to 80.119% when pulling 4d4ca9a on feature/apiv3-test-support into e68ca92 on develop.

Copy link
Collaborator

@robingustafsson robingustafsson left a comment

Choose a reason for hiding this comment

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

Looking good, added some comments regarding general UX of output


stream = test_run.result_stream([m.str_raw(True) for m in metrics])

click.echo(pprint_header(metrics))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should wait with printing the header until we get the first results and instead print "Initializing test..." or equivalent until the first results arrive (maybe even print the "live feedback" messages until we reach the "running" state).

Return a pretty printed string with the header of the metric streaming
output, consisting of the name of the metrics including the parameters.
"""
return u'\t'.join([u'TIMESTAMP:'] + [u'{0}:'.format(m.str_ui(True)) for m in returned_metrics])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there some way to make the header columns better align with the position of the result rows? Would it be bad to reserve a fixed space for each column?

Also could we perhaps insert the header every 20 or so result rows, not to forget what each column represents. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly went with using a simple \t between columns as it seemed to provide consistency with the rest of the commands's output, but at the expense of readability indeed! I will play a bit with the aligning, as I fully agree it would be a welcome and doable improvement.


@test.command('list', short_help='List tests.')
@click.option('--project_id', 'project_ids', multiple=True, help='Id of the project to list tests from.')
def list_tests(project_ids):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we limit this to the 10-20 or so tests with the most recent test runs and then add a line of output saying how to print/list more tests (using an --all or --limit flag perhaps)? This would greatly enhance the UX for someone like me who has 100s of tests across orgs and projects 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Did not account for such hard core users, but it does make much sense.

for id_ in set(project_ids):
tests.extend(client.list_tests(project_id=id_))

click.echo("ID:\tNAME:\tLAST RUN DATE:\tLAST RUN STATUS:\tCONFIG:")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as comment below regarding fixed column width, is that possible? Saying for example we show the first 32 chars of the test name and then add "...", maybe have a flag --full to show full information?

Add a `--limit` argument (defaulting to 20) that controls the number of
tests that are displayed during `test list`.
Fix PEP8 issues and a small bug on userscenario.
@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Coverage increased (+6.7%) to 80.276% when pulling eaae558 on feature/apiv3-test-support into e68ca92 on develop.

Diego M. Rodriguez added 3 commits May 23, 2017 15:09
Change the output of the "test run" command, showing an initialization
message once the test run is requested and delay printing the header
until the actual metrics start to arrive, additionally printing them
every 20 lines for clarity.
Update docs.
Use fixed widths for the columns on the output of the `test list`
command, introducing a `util.ColumnFormatter` helper and aiming for
reasonable defaults for each column's width. The previous behaviour
(using just '\t' as a separator between each column, with no truncating
or further processing of the cells) is preserved by introducing a
`--full_width` flag.
Use fixed with and truncation (optionally disabled by `--full_width`)
for the `test run` command, analogously to the output of `test list`.
@coveralls
Copy link

Coverage Status

Coverage increased (+7.5%) to 81.006% when pulling ad82357 on feature/apiv3-test-support into e68ca92 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.5%) to 81.006% when pulling 9b6eea8 on feature/apiv3-test-support into e68ca92 on develop.

1 similar comment
@coveralls
Copy link

coveralls commented May 24, 2017

Coverage Status

Coverage increased (+7.5%) to 81.006% when pulling 9b6eea8 on feature/apiv3-test-support into e68ca92 on develop.

@coveralls
Copy link

coveralls commented May 24, 2017

Coverage Status

Coverage increased (+7.6%) to 81.146% when pulling c00e6e0 on feature/apiv3-test-support into e68ca92 on develop.

@robingustafsson robingustafsson merged commit 3e7a8bd into develop Oct 3, 2017
@robingustafsson robingustafsson deleted the feature/apiv3-test-support branch October 27, 2017 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants