Skip to content

[RDBMS] Add Test Scenarios#3240

Merged
tjprescott merged 3 commits intoAzure:masterfrom
Suna-MS:add_tests
May 11, 2017
Merged

[RDBMS] Add Test Scenarios#3240
tjprescott merged 3 commits intoAzure:masterfrom
Suna-MS:add_tests

Conversation

@Suna-MS
Copy link
Copy Markdown
Contributor

@Suna-MS Suna-MS commented May 8, 2017


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • The PR has modified HISTORY.rst with an appropriate description of the change (see Modifying change log).

Command Guidelines

  • Each command and parameter has a meaningful description.
  • Each new command has a test.

(see Authoring Command Modules)

@msftclas
Copy link
Copy Markdown

msftclas commented May 8, 2017

@Suna-MS,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@tjprescott tjprescott changed the title Add test cases for mysql and postgrs command module [RDBMS] Add Test Scenarios May 8, 2017
@tjprescott
Copy link
Copy Markdown
Member

These have been run live successfully against the staging environment. This PR will be updated with the test recordings once the production environment is live.

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 8, 2017

Codecov Report

Merging #3240 into master will increase coverage by 0.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3240      +/-   ##
==========================================
+ Coverage   70.44%   70.68%   +0.23%     
==========================================
  Files         391      391              
  Lines       25130    25130              
  Branches     3815     3815              
==========================================
+ Hits        17702    17762      +60     
+ Misses       6312     6243      -69     
- Partials     1116     1125       +9
Impacted Files Coverage Δ
...-rdbms/azure/cli/command_modules/rdbms/commands.py 100% <0%> (+10.2%) ⬆️
...cli-rdbms/azure/cli/command_modules/rdbms/_util.py 65.95% <0%> (+17.02%) ⬆️
...dbms/azure/cli/command_modules/rdbms/validators.py 74.19% <0%> (+45.16%) ⬆️
...li-rdbms/azure/cli/command_modules/rdbms/custom.py 65.57% <0%> (+54.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30776ec...f4316a2. Read the comment docs.

@tjprescott tjprescott added this to the Sprint 16 milestone May 8, 2017
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will resolve in the commit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The name is too generic.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If deleting the resource group will delete the server, make skip_delete to be True actually saves time during live test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

engine is a generic name, it is recommended to be a more specific name. for example, database_engine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use addCleanup instead to make sure the file is deleted even if the assertion fails. https://docs.python.org/2/library/unittest.html#unittest.TestCase.addCleanup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just disable the too-many-arguments rule project-wise. So you don't need this anymore.

tjprescott
tjprescott previously approved these changes May 9, 2017
@troydai
Copy link
Copy Markdown
Contributor

troydai commented May 9, 2017

Taking a second look.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I may have missed something obvious but template has one parameter but 2 params are passed to template.format(...).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed that dict.get will not throw KeyError exception so removed the statement.

troydai
troydai previously approved these changes May 10, 2017
@Suna-MS Suna-MS dismissed stale reviews from troydai and tjprescott via f4316a2 May 11, 2017 05:44
@tjprescott tjprescott merged commit b397cd1 into Azure:master May 11, 2017
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.

7 participants