Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[1LP][RFR] - Adding test to login new user using upper case password in the existing testcase #10105

Conversation

dgaikwad
Copy link
Contributor

@dgaikwad dgaikwad commented May 10, 2020

adding_test Test to login new user and user having password capital case character in the existing testcase and removed test_authorized_users_can_login

Purpose or Intent

PRT Run

{{pytest: cfme/tests/integration/test_db_auth.py -k "test_db_user_pwd" --long-running -v}}

@dgaikwad dgaikwad changed the title [WIPTEST] - Adding test to login SSUI using upper case password [WIPTEST] - Adding test to login new user using upper case password May 10, 2020
@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch from c6ef8d9 to 5f0c895 Compare May 10, 2020 19:33
@dgaikwad dgaikwad changed the title [WIPTEST] - Adding test to login new user using upper case password [RFR] - Adding test to login new user using upper case password May 10, 2020
Comment on lines 2579 to 2611
user, _ = user_self_service_role

# login with user having self service role and uppercase password
with user:
appliance.server.login(user)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like how neatly this has been implemented, but if the user creation is a part of the test, I think it will be better if you put user creation within the test and not as a part of setup i.e. don't use fixture to create user.

Also it kinda looks like it should be a part of smoke test, thoughts on adding a smoke test marker?

Copy link
Contributor

Choose a reason for hiding this comment

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

@valaparthvi I think it's fine having it in the fixture. I think creating the user is really a part of the setup, and the point of the test is ensuring that the user is able to login. That said, I'd be fine if you want to include the user creation as part of the test, so up to you @dgaikwad.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@valaparthvi @john-dupuy I have added user creation inside test and also put a smoke marker.

@valaparthvi valaparthvi added the test-automation To be applied on PR's which are automating existing manual cases label May 11, 2020
@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch from 5f0c895 to cd3bcdb Compare May 13, 2020 07:14
@dgaikwad dgaikwad changed the title [RFR] - Adding test to login new user using upper case password [WIPTEST] - Adding test to login new user using upper case password May 13, 2020
@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch 2 times, most recently from 8f0b8f1 to 83ac65f Compare May 13, 2020 07:16
@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch from 83ac65f to e843dbf Compare May 13, 2020 09:08
@dgaikwad dgaikwad changed the title [WIPTEST] - Adding test to login new user using upper case password [RFR] - Adding test to login new user using upper case password May 13, 2020
@valaparthvi valaparthvi changed the title [RFR] - Adding test to login new user using upper case password [1LP][RFR] - Adding test to login new user using upper case password May 14, 2020
@@ -2577,7 +2577,38 @@ def test_authorized_users_can_login():
1. User created
2. User logged in
"""
pass
# creating role
Copy link
Member

Choose a reason for hiding this comment

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

I believe we already have automated coverage for this test.

https://github.com/ManageIQ/integration_tests/blob/master/cfme/tests/integration/test_db_auth.py#L26

test_db_auth is creating a user with a credential randomized by fauxfactory.gen_alpha, which will include lower and uppercase characters.

It doesn't always include an uppercase character, as its randomized.

I think the existing test cases and fixture should be modified to always include a capital letter in the credential, than create a new test case for this.

@mshriver mshriver changed the title [1LP][RFR] - Adding test to login new user using upper case password [1LP][WIP] - Adding test to login new user using upper case password May 15, 2020
@dajoRH dajoRH added the WIP label May 15, 2020
Copy link
Member

@mshriver mshriver left a comment

Choose a reason for hiding this comment

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

I think the automated test coverage already exists, please adjust existing test case and remove this duplicate case.

@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch 2 times, most recently from 3d8eba6 to 3704d44 Compare May 18, 2020 07:43
@dgaikwad dgaikwad changed the title [1LP][WIP] - Adding test to login new user using upper case password [1LP][WIPTEST] - Adding test to login new user using upper case password May 18, 2020
@dgaikwad dgaikwad force-pushed the test_authorized_users_can_login_password_upper branch from 3704d44 to 9fccdd4 Compare May 18, 2020 07:55
@dajoRH
Copy link
Contributor

dajoRH commented May 18, 2020

I detected some fixture changes in commit 9fccdd4

The local fixture nonexistent_user is used in the following files:

  • cfme/tests/integration/test_db_auth.py
    • test_login_invalid_user

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@dajoRH dajoRH added WIP-testing and removed WIP labels May 18, 2020
@dgaikwad dgaikwad changed the title [1LP][WIPTEST] - Adding test to login new user using upper case password [1LP][RFR] - Adding test to login new user using upper case password in the existing testcase May 18, 2020
@dgaikwad dgaikwad requested a review from mshriver May 18, 2020 08:53
Copy link
Member

@mshriver mshriver left a comment

Choose a reason for hiding this comment

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

Perfect! Great job using existing parametrization to get the coverage.

@mshriver mshriver merged commit 88c4f4b into ManageIQ:master May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lint-ok test-automation To be applied on PR's which are automating existing manual cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants