qxf2 / qxf2-page-object-model

Write Selenium and Appium tests in Python using the Page Object pattern. This Pythonic GUI and API test automation framework will help you get started with QA automation quickly. It comes with many useful integrations like - email, BrowserStack, Slack, TestRail, etc. This repository is developed and maintained by Qxf2 Services.
https://qxf2.com
MIT License
256 stars 191 forks source link

Integrate questionary #297

Closed akkuldn closed 3 years ago

akkuldn commented 3 years ago

This is a new feature that allows the user to run the tests interactively via the terminal. Steps to run: 1) If you're using Gitbash for windows please make sure to set your bash alias by adding the following command to your bash_rc file alias python='winpty python.exe' 2) Install the requirements using pip install -r reqquirements.txt 3) Commands to run the test : python -m pytest tests/test_example_form.py --interactivemode_flag Y python -m pytest tests/test_mobile_bitcoin_price.py --interactivemode_flag Y python -m pytest tests/test_api_example.py --interactivemode_flag Y

Changes made:

PreedhiVivek commented 3 years ago

@akkuldn An interesting and useful feature add-on to our framework. Nice Akkul! My observations:

akkuldn commented 3 years ago

@akkuldn An interesting and useful feature add-on to our framework. Nice Akkul! My observations:

  • Errors out while selecting the firefox version, when I want to change default options: browser_version_error
  • Do you want to run the test with a default set of options? No -> What would you like to change? Exit -> Errors out. Would be good to go back to the default options question on Exit. error_on_exit
  • askquestions<>() functions will have to be unit tested well, while modifying different default options and ensuring expected behaviour.
  • In Readme.md '(This will give the user a menu of different choices allowing the user to pick their desired configuration to run the test)' can be rephrased to (This option will allow the user to pick the desired configuration to run the test, from the menu displayed)' and typo aliash. Is that Note even required? Isn't that understood and holds good for running the test suit with any other option as well. You can probably make it a common note under that section?
  • conftest.py Watch out for line spacing. Could see many empty lines and very long lines of code. I suggest, running your modified files against pylint and radon. You could fix the ones that belong to your change, if all is too much or parked as a separate task.
  • API_Player.py remove commented out code line # 14, 26, 27
  • Interactive_mode.py, functions such as ask_questions_gui() and ask_questions_mobile() has be broken down. They are too long. Also, ensure this new module is pylint and radon compliant.
  • Commits will have to be squashed before merge.
  • I will be running through the API test changes and will update if I find any comments.

Thanks for the feedback Preedhi, 1) Fixed 2) I added the exit option intentionally so that the user could exit out of the while loop incase he doesn't want to run the test 3) Added unit tests, you can find it inside the tests directory test_interactive_mode.py 4) Fixed 5) Fixed 6) Fixed 7) Fixed 8) Ill squash the commits once I get the approval to merge

Please do let me know of any further comments

PreedhiVivek commented 3 years ago

@akkuldn My re-reviewing comments: Readme file:

conftest.py

  • Errors out while selecting the firefox version, when I want to change default options: browser_version_error

Test runs smoothly if I change browser to Firefox. However, when I tried Firefox+Windows+10 combination, observed that Firefox browser launches and the cursor keeps blinking at the address bar. In the console, Errros out when I select Firefox, Windows, version 10 Exception when trying to run test: C:\Users\PreedhiVivek\code\POM\qxf2-page-object-model\conftest.py Python says:Message: permission denied

  • askquestions<>() functions will have to be unit tested well, while modifying different default options and ensuring expected behaviour.

Unit tests should ensure all such combinations are covered. Please ensure that.

  • Do you want to run the test with a default set of options? No -> What would you like to change? Exit -> Errors out. Would be good to go back to the default options question on Exit. error_on_exit

I understand the use-case now Akkul. But the handling of this case should be looked into. It currently errors out when Exit is chosen from the menu. Maybe, catch the exception and log a good message.

  • Interactive_mode.py, functions such as ask_questions_gui() and ask_questions_mobile() has be broken down. They are too long. Also, ensure this new module is pylint and radon compliant.

I do see that, you have broken down code well now. However, it has to be done a bit more. Pylint of this module is rated at 7.72/10 and radon run also points out few functions rated B/C which need to be addressed.

akkuldn commented 3 years ago

@akkuldn My re-reviewing comments: Readme file:

  • Remove empty line # 152, _Note:_ to NOTE: can be changed to be in sync with existing convention and would be good to have interactive_mode as the optional command line parameter in-place of interactive_mode_flag

conftest.py

  • unwanted print line # 97
  • Errors out while selecting the firefox version, when I want to change default options: browser_version_error

Test runs smoothly if I change browser to Firefox. However, when I tried Firefox+Windows+10 combination, observed that Firefox browser launches and the cursor keeps blinking at the address bar. In the console, Errros out when I select Firefox, Windows, version 10 Exception when trying to run test: C:\Users\PreedhiVivek\code\POM\qxf2-page-object-model\conftest.py Python says:Message: permission denied

  • askquestions<>() functions will have to be unit tested well, while modifying different default options and ensuring expected behaviour.

Unit tests should ensure all such combinations are covered. Please ensure that.

  • Do you want to run the test with a default set of options? No -> What would you like to change? Exit -> Errors out. Would be good to go back to the default options question on Exit. error_on_exit

I understand the use-case now Akkul. But the handling of this case should be looked into. It currently errors out when Exit is chosen from the menu. Maybe, catch the exception and log a good message.

  • Interactive_mode.py, functions such as ask_questions_gui() and ask_questions_mobile() has be broken down. They are too long. Also, ensure this new module is pylint and radon compliant.

I do see that, you have broken down code well now. However, it has to be done a bit more. Pylint of this module is rated at 7.72/10 and radon run also points out few functions rated B/C which need to be addressed.

@PreedhiVivek , 1) Removed line and changed "Note" to Note. Regarding the "interactive_mode" part, I had it as 'interactive_mode' earlier but Avinash had suggested to use "interactive_mode_flag " as it followed the naming convention. 2) removed unwanted print 3) I could not replicate the issue. But however, os_name, browser_versions, Os_version is used only when running the test remotely. So i have made changes to the code such that you wont be able to select the OS Name or browser version unless your remote_flag is not set to "Yes". Adding more unit test scenarios would be bit of a hassle as it requires human interaction with terminal for each test, and it requires to change the test file for every test case manually 4) Added message for the exit option 5) Improved the code, now getting pyling rating of 9.05, now there are four functions that show b,c rating in radon, i can't break those functions any smaller as they are nested if statements

PreedhiVivek commented 3 years ago

@akkuldn I am good with your changes. One final formatting suggestion in Readme, ReadMe_currently To improve readability, ReadMe_spacing

akkuldn commented 3 years ago

@akkuldn I am good with your changes. One final formatting suggestion in Readme, ReadMe_currently To improve readability, ReadMe_spacing

Made the change @PreedhiVivek