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

Reorganize test run in automation #1624

Merged
merged 3 commits into from
Jan 5, 2017
Merged

Reorganize test run in automation #1624

merged 3 commits into from
Jan 5, 2017

Conversation

troydai
Copy link
Contributor

@troydai troydai commented Dec 31, 2016 •
edited
Loading

This pull request is the first step towards consolidating automation scripts. Changes include:

  1. Use nosetests to execute the tests in automation. Nosetests can run tests in parallel. It also generates xunit like test report in XML, which can be feed into other reporting systems.
  2. Consolidate automation related to into automation package and remove duplicate and out-of-date code.
  3. Clean up and add comments.
  4. Add basics code coverage facility. It now generates code coverage data. The accuracy of the results needs further study.

@troydai troydai assigned troydai and unassigned derekbekoe Jan 3, 2017
tjprescott
tjprescott reviewed Jan 3, 2017
View reviewed changes
Copy link
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

A few questions but I like that we're moving toward running (and recording!) tests in parallel.

requirements.txt
@@ -2,6 +2,7 @@ adal==0.4.3
applicationinsights==0.10.0
argcomplete==1.3.0
colorama==0.3.7
coverage==4.2
Copy link
Member

Choose a reason for hiding this comment

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

Does this file only apply to dev-setup or all installation methods? It would seem that if someone did a curl install of the CLI for production use, then they wouldn't need (or probably want) packages related strictly to testing and development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for test only. Where do we list the requirements for dev environment?

Copy link
Member

Choose a reason for hiding this comment

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

There's a requirements.txt file in the CLI's root directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I don't think this requirement.txt should impact the user dependencies which are defined in the setup.py as far as I know. @derekbekoe

Copy link
Member

Choose a reason for hiding this comment

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

And I don't think this requirement.txt should impact the user dependencies which are defined in the setup.py as far as I know. @derekbekoe

Correct.

scripts/automation/coverage/run.py
get_command_modules_paths_with_tests, get_repo_root, get_test_results_dir, make_dirs


# TODO: Fix track command logic in vcr_test_base.py.
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this is the traditional line or branch based code coverage metric, or is it a port of the command coverage logic (which needs to be augmented, but that's a separate thing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the code I ported from the command coverage logic. In my test, the command coverage doesn't work. I may not have the full context so I add this comment for following up.

Copy link
Member

Choose a reason for hiding this comment

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

So the code is in place but not actively being used right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It is not actively being used.

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/tests/test_vm_commands.py
@@ -1064,6 +1064,9 @@ def body(self):
user_name = 'myadmin'
config_file = _write_config_file(user_name)

if not os.path.exists(config_file):
Copy link
Member

Choose a reason for hiding this comment

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

Is this related to the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. This is not related. I'll remove it.

derekbekoe
derekbekoe reviewed Jan 4, 2017
View reviewed changes
requirements.txt
@@ -2,6 +2,7 @@ adal==0.4.3
applicationinsights==0.10.0
argcomplete==1.3.0
colorama==0.3.7
coverage==4.2
Copy link
Member

Choose a reason for hiding this comment

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

And I don't think this requirement.txt should impact the user dependencies which are defined in the setup.py as far as I know. @derekbekoe

Correct.

scripts/automation/coverage/run.py
run_nose(name, test_path)

import shutil
shutil.move('.coverage', os.path.join(test_results_folder, '.covarege.{}'.format(name)))
Copy link
Member

Choose a reason for hiding this comment

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

typo? 'covarege'.
Not sure if this affects the logic of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a type. I'll fix it.

scripts/automation/coverage/run.py

class CoverageContext(object):
def __init__(self):
self._cov = Coverage(cover_pylib=False)#data_suffix='azure', cover_pylib=False)
Copy link
Member

Choose a reason for hiding this comment

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

delete the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@tjprescott tjprescott merged commit f564c90 into Azure:master Jan 5, 2017
@troydai troydai deleted the troy-nose branch January 5, 2017 22:17
thegalah pushed a commit to thegalah/azure-cli that referenced this pull request Jan 6, 2017
Merge remote-tracking branch 'Azure/master'
f0b2b46 
* Azure/master:
  Fix PEP8 for azure-cli (Azure#1661)
  [VM/VMSS] Enable Multi-Cloud VM/VMSS Create (Azure#1654)
  Fix blob type validator (Azure#1656)
  Improve @file handling logic (Azure#1648)
  Name private_config.json differently in VM test (Azure#1653)
  Update regex of parsing sdk function argument comments (Azure#1650)
  Reorganize test run in automation (Azure#1624)
  Fix issue 1644. (Azure#1647)
  [Network] VNet Gateway Fixes and Enhancements (Azure#1643)
  [Network] Application Gateway Commands and Fixes (Azure#1606)
  [Docs] Correcting unittest location in docs (Azure#1642)
  [ACS] Add a table transform for acs show to match acs list. (Azure#1641)
  [ACS] Add a unit test and fix a bug with az browse ... (Azure#1640)
  [Component] Fix pip seg fault on 'az component update' (Azure#1639)
  fixed typo (Azure#1634)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjprescott tjprescott tjprescott approved these changes

@derekbekoe derekbekoe derekbekoe approved these changes

Assignees

@troydai troydai

Labels
cla-not-required Test Framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@troydai @tjprescott @derekbekoe @azurecla @haroldrandom