1
\$\begingroup\$

I did this test for a job and the feedback was:

"while many were added to test private methods, they did not do anything to ensure the public interface worked correctly, edge cases were missed"

But I thought I covered the public interface and edge cases.

Any other improvements I could have made would be appreciated too.

Thanks

# Given a words.txt file containing a newline-delimited list of dictionary
# words, please implement the Anagrams class so that the get_anagrams() method
# returns all anagrams from words.txt for a given word.
#
# Requirements:
#   - Optimise the code for fast retrieval
#   - Thread Safe implementation
#   - Write more tests

import unittest
from unittest.mock import patch, MagicMock
import itertools

class Anagrams:
    def __init__(self, filename):
        self.filename = filename
        self.map = {}

    def _create_word_list(self):
        return open(self.filename).read().splitlines()

    def initialise(self):
        # This method is to make testing easier (as opposed to creating the map in __init__).
        # (we shouldn't need words.txt to exist to be able to test the methods
        # unrelated to reading the file
        self.words = self._create_word_list()
        self.map = self._create_map()

    def _create_word_key(self, word):
        abbr = sorted(word)
        return ''.join(abbr)

    def _create_map(self):
        word_map = {}
        for word in self.words:
            key = self._create_word_key(word)
            word_map.setdefault(key, []).append(word)

        return word_map

    def get_anagrams(self, word):
        key = self._create_word_key(word)
        try:
            return self.map[key]

        except KeyError:
            print('Word is not in dictionary')



# Integration tests
class TestAnagrams(unittest.TestCase):
    def test_anagrams(self):
        anagrams = Anagrams('words.txt')
        anagrams.initialise()
        self.assertEqual(anagrams.get_anagrams('plates'), ['palest', 'pastel', 'petals', 'plates', 'staple'])
        self.assertEqual(anagrams.get_anagrams('eat'), ['ate', 'eat', 'tea'])

    @patch('builtins.print')
    def test_anagrams_not_exist(self, mock_print):
        anagrams = Anagrams('words.txt')
        anagrams.initialise()

        anagrams.get_anagrams('abcdef')

        mock_print.assert_called_once_with('Word is not in dictionary')


# Unit tests
class TestAnagramsMethods(unittest.TestCase):
    def test_create_word_key(self):
        words = ['palest', 'pastel', 'petals', 'plates', 'staple']
        anagrams = Anagrams('fakefilename')

        results = [] 
        for word in words:
            key = anagrams._create_word_key(word)
            results.append(key)

        expected = 'aelpst'
        for result in results:
            self.assertEqual(result, expected)


    def test_create_map(self):
        anagrams = Anagrams('fakefilename')
        anagrams._get_word_key = MagicMock(side_effect=['key1', 'key1', 'key1', 'key2', 'key2', 'key2'])
        anagrams.words = ['palest', 'pastel', 'plates', 'ate', 'eat', 'tea']

        result = anagrams._create_map()

        expected = {'aelpst': ['palest', 'pastel', 'plates'],
                    'aet': ['ate', 'eat', 'tea']}

        self.assertEqual(result, expected)


    @patch('builtins.open')
    def test_create_word_list(self, mock_open):
        mock_open.return_value.read.return_value = 'one\ntwo\nthree\n'

        anagrams = Anagrams('fakefilename')

        result = anagrams._create_word_list()

        self.assertEqual(result, ['one', 'two', 'three'])


    def test_initialise(self):
        anagrams = Anagrams('fakefilename')
        anagrams._create_word_list = MagicMock(return_value='word_list')
        anagrams._create_map = MagicMock(return_value='map')

        anagrams.initialise()

        anagrams._create_map.assert_called_once()
        anagrams._create_word_list.assert_called_once()

        self.assertEqual(anagrams.words, 'word_list')
        self.assertEqual(anagrams.map, 'map')


class TestGetAnagrams(unittest.TestCase):
    def test_existing_word(self):
        anagrams = Anagrams('fakefilename')
        anagrams._create_word_key = MagicMock(return_value='key')
        anagrams.map = {'key': ['anagram1', 'anagram2']}

        result = anagrams.get_anagrams('word')

        self.assertEqual(result, ['anagram1', 'anagram2'])

    @patch('builtins.print')
    def test_incorrect_word(self, mock_print):
        anagrams = Anagrams('fakefilename')
        anagrams._get_word_key = MagicMock(return_value='key')
        anagrams.map = {'notkey': ['anagram1', 'anagram2']}

        result = anagrams.get_anagrams('word')

        mock_print.assert_called_once_with('Word is not in dictionary')


if __name__ == '__main__':
    unittest.main()
\$\endgroup\$
  • 1
    \$\begingroup\$ Did the feedback not inform you about any missed edge-cases? That sounds like rather terrible feedback to me. \$\endgroup\$ – Gloweye Oct 2 at 9:56
  • 1
    \$\begingroup\$ Because of your description I added the interview-questions tag. Feel free to remove it if you don't deem it appropriate. \$\endgroup\$ – AlexV Oct 2 at 10:02
  • \$\begingroup\$ I guess I should have also done the following (?): 1) returned a helpful error message if a non-string argument was given to get_anagrams() and created a unit test for this 2) Created unit test for get_anagrams() with empty string argument (although I think this is the same as the unit test test_incorrect_word()) 3) Created a unit test for when initialise() is not called first (so self.map doesn't exist). However I think this is also the same as test_incorrect_word() \$\endgroup\$ – Jonny Shanahan Oct 2 at 11:08
  • \$\begingroup\$ @Gloweye Absolutely. Unfortunately, that appears to be somewhat common nowadays. \$\endgroup\$ – Mast Oct 2 at 19:04
  • \$\begingroup\$ Can you add more context about what the code is supposed to do? If this is a programming challenge/interview question, can you include the prompt and your own explanation of how you solved it? \$\endgroup\$ – Dannnno Oct 3 at 13:46
1
\$\begingroup\$

For an interview, I would test everything very thoroughly.

The public interface consists of Anagrams(), Anagrams.initialise(), and Anagrams.get_anagram(). The public interface also includes the fact that initialise() needs to be called before get_anagram() and the format of text file.

So test everything:

For Anagram(filename): (some of these errors don't occur until initialise() is called)

  • no filename
  • empty filename
  • bad filename (bad characters in the filename)
  • bad file permissions

For initialise()

  • call get_anagrams() without callinginitialize()` first
  • was the file opened?
  • did the whole file get read?
  • was the file closed? (No)
  • bad format of the file (multiple words per line; blank lines; with/without \n at the end; words with hyphens, apostrophes, or other unusual characters, ...)

For get_anagrams(word):

  • called with no word
  • called with empty word
  • called with really long word
  • called with word that has spaces, control characters, digits, uppercase, lowercase, apostrophes, hyphens, and/or strange unicode characters
\$\endgroup\$
1
\$\begingroup\$

This mostly a response to your comment, but I feel it really shouldn't be another comment.

I guess I should have also done the following (?): 1) returned a helpful error message if a non-string argument was given to get_anagrams() and created a unit test for this 2) Created unit test for get_anagrams() with empty string argument (although I think this is the same as the unit test test_incorrect_word()) 3) Created a unit test for when initialise() is not called first (so self.map doesn't exist). However I think this is also the same as test_incorrect_word()

I disagree on all those cases.

  1. I would expect to get raised a TypeError in this case. A wrong argument is an exceptional case, and exceptions are the best way to handle it. Arguably, getting a non-string here is a programming error, though in rare cases it might be a user error.
  2. This should be able to be handled perfectly fine with an ordinary response. The response to any string is a list of anagrams. Since there are no anagrams to an empty string, a list with just an empty string is the correct response, just like the response to any other word without anagram is just a list containing that word.
  3. You should still do that work in __init__. However, you might want to supply a special testing file upon instantiation for tests, containing just the anagrams you test on. For tests that don't care about reading the file, you can give it an empty file. You can import from the tempfile module for this. You can also hardcode the contents of your testing file, dump it into a tempfile, and use that for testing the tests that DO need a file.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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