2
\$\begingroup\$

I'd like some constructive feedback on the code that I have written to test the API for smartsh33t.com I wrote it in Python.

I wrote the program to take .json files and read them into the testrunner. then invoke the companies API for the create sheet functions, then record the results.

I am interested in general structural/design/architecture improvements and other feedback to improve future work.

#! usr/bin/env python3

import json
import logging
import smartsheet
import smartsheet.exceptions
import smartsheet.models
import access_token

import urllib.error

# logger setup
logging.basicConfig(filename="err_log.txt", level=logging.ERROR, format=' %(asctime)s - %(levelname)s - %(message)s')
open("err_log.txt", "w")

# Initialize client, actual access token kept in local file only.
smartsheet_client = smartsheet.Smartsheet(access_token.token)

# Make sure we don't miss any errors
smartsheet_client.errors_as_exceptions(True)


class TestRunner:
    passed: int
    failed: int
    test_count: int

    # initial test setup
    def __init__(self):
        self.passed = 0
        self.failed = 0
        self.test_count = 0
        # sheet 0 happy path test while sheets 1 & 2 throw errors
        self.tests = ["Testrc/sheet0.json", "Testrc/sheet1.json", "Testrc/sheet2.json"]
        self.json_files_to_test(self.tests)

    # function to iterate through the files.
    def json_files_to_test(self, json_tests):
        for test in json_tests:
            self.prepare_json_file(test)
        # printing the pass/fail report for the tests.
        print('\n\nTest Report')
        print('====================================================')
        print('tests passed = ' + str(self.passed))
        print('tests failed = ' + str(self.failed))
        print('percent of tests passing = ' + str(100*(self.passed/self.test_count)) + "%")
        print('total number of tests = ' + str(self.test_count))
        print('====================================================')

    # load json files to be tested and then the tests to be run.
    def prepare_json_file(self, test):
        with open(test, "r+") as read_file:
            read_json: object = json.load(read_file)
            spec_sheet = smartsheet.models.Sheet(read_json)
            self.create_test(smartsheet_client, spec_sheet)
            self.create_in_folder_test(smartsheet_client, spec_sheet)
            self.create_in_workspace_test(smartsheet_client, spec_sheet)

    # sends a minimal sheet to be created in the Default folder(Home)
    def create_test(self, ss_client, spec_sheet):
        self.test_count += 1
        try:
            response = ss_client.Home.create_sheet(spec_sheet)
            self.record_success(response)
        except (smartsheet.exceptions.ApiError, urllib.error.HTTPError) as e:
            logging.exception(e)
            self.handle_exception(e, spec_sheet)

    # sends a minimal sheet to be created in a folder by id
    def create_in_folder_test(self, ss_client, spec_sheet):
        self.test_count += 1
        # found id via curl -X GET -H "Authorization: Bearer ttctt6u346l1wya2oxc0g80g2k"
        # "https://api.smartsheet.com/2.0/home/folders"
        folder_id = 2843491320522628
        try:
            response = ss_client.Folders.create_sheet_in_folder(folder_id, spec_sheet)
            self.record_success(response)
        except (smartsheet.exceptions.ApiError, urllib.error.HTTPError) as e:
            logging.exception(e)
            self.handle_exception(e, spec_sheet)

    # sends a minimal sheet to be created in a workspace by id
    def create_in_workspace_test(self, ss_client, spec_sheet):
        self.test_count += 1
        # found id via curl -X GET -H "Authorization: Bearer vpj1fg3b92qlorn6yibodw2w0u"
        # "https://ai.smartsheet.com/2.0/workspaces"
        workspace_id = 7730152571463556
        try:
            response = ss_client.Workspaces.create_sheet_in_workspace(workspace_id, spec_sheet)
            self.record_success(response)
        except (smartsheet.exceptions.ApiError, urllib.error.HTTPError) as e:
            logging.exception(e)
            self.handle_exception(e, spec_sheet)

    def record_success(self, response):
        parsed_json = json.loads(str(response))
        print(parsed_json["result"]["name"] + " passed in the default instance")
        self.passed += 1

    def handle_exception(self, e, spec_sheet):
        parsed_spec_sheet = json.loads(str(spec_sheet))
        parsed_err = json.loads(str(e))
        print(parsed_spec_sheet["name"] + "\nfailure = " + parsed_err["result"]["name"] + " " + str(
            parsed_err["result"]["errorCode"]))
        print(json.dumps(parsed_err, sort_keys=True, indent=4))
        self.failed += 1

    def print_test_results(self):
        print('\n\n Test Report')
        print('====================================================')
        print('tests succeed = ' + str(self.passed))
        print("tests failed = " + str(self.failed))
        print('percent of tests passing = ' + str(100 * (self.passed / self.test_count)) + "%")
        print('total number of tests = ' + str(self.test_count))
        print('====================================================')


if __name__ == "__main__":
    TestRunner()
\$\endgroup\$
3
  • 2
    \$\begingroup\$ I sincerly hope this is not your real API token or that the token does not need to be confidential... \$\endgroup\$
    – AlexV
    Commented Jul 10, 2019 at 18:45
  • \$\begingroup\$ @AlexV I recall editing it to not be valid, however, it has now been removed. \$\endgroup\$
    – Dan
    Commented Jul 17, 2019 at 20:54
  • \$\begingroup\$ @AlexV do you have any other feedback or critics? I can take it. \$\endgroup\$
    – Dan
    Commented Jul 26, 2019 at 22:39

1 Answer 1

3
\$\begingroup\$

I have no experience with smartsheet or their API, so there only will be general remarks regarding Python and testing.


Use a testing framework

You have started to write your own testing framework. Don't! There are a lot of battletested Python test frameworks out there that do all the tedious work for you, e.g. finding test cases, running them, and collecting their results in an appropriate, usually also configurable manner.

Just to name the most common ones*:

  • unittest: comes directly with Python
  • nose/nose2: an external library available through pip, which claims to "extend unittest to make testing easier". nose2 is an extension of nose which claims to make testing even easier
  • pytest: has a different flavor from the previous two, but also does the job quite good. This is what I actually use if I write tests.

I invite you to have a look at all of them, where unittest and nose can likely be treated as equivalent with regard to the way they structure text. There is are a lot of different blog posts on that topic, e.g. on RealPython or PYTHON TESTING, that show them either in comparison or separately.

It might not make a big difference for what you want to do at the moment, but will make all the difference once you start to have more tests.

Use string formatting

If you bring together static text with variables to create dynmic output, it's usually better to use string formatting instead of manually str(...)inging all the variables and then concatenate them together. If you are working with Python 3.6 or higher, you can use f-strings to do this. An example from your code rewritten using f-strings:

def print_test_results(self):
    print('\n\n Test Report')
    print('====================================================')
    print(f'tests succeed = {self.passed}')
    print(f'tests failed = {self.failed}')
    print(
        f'percent of tests passing = {100 * (self.passed / self.test_count)}%'
    )
    print(f'total number of tests = {self.test_count}')
    print('====================================================')

There is also an Real Python article on this topic where the classical ways using % and str.format(...) are compared with the new f-strings.

Don't just open files

When you set up the logger, you do this:

# logger setup
logging.basicConfig(filename="err_log.txt", level=logging.ERROR, format=' %(asctime)s - %(levelname)s - %(message)s')
open("err_log.txt", "w")

There is no need to do this and you also propably should not do it. open("err_log.txt", "w") likely has no effect whatsoever on your program. logging will open the file itself before starting to write to it. Maybe it has even done so at that point. Apart from that it's good practice to use open(...) in conjuction with the with-statement to make sure the file gets closed properly even in the case of an exception.


*Based on my experience while researching that topic quite a while ago.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.