{"action":"created","issue":{"url":"https://api.github.com/repos/bear/parsedatetime/issues/196","repository_url":"https://api.github.com/repos/bear/parsedatetime","labels_url":"https://api.github.com/repos/bear/parsedatetime/issues/196/labels{/name}","comments_url":"https://api.github.com/repos/bear/parsedatetime/issues/196/comments","events_url":"https://api.github.com/repos/bear/parsedatetime/issues/196/events","html_url":"https://github.com/bear/parsedatetime/issues/196","id":176183595,"number":196,"title":"Revised testing architecture","user":{"login":"idpaterson","id":507058,"avatar_url":"https://avatars.githubusercontent.com/u/507058?v=3","gravatar_id":"","url":"https://api.github.com/users/idpaterson","html_url":"https://github.com/idpaterson","followers_url":"https://api.github.com/users/idpaterson/followers","following_url":"https://api.github.com/users/idpaterson/following{/other_user}","gists_url":"https://api.github.com/users/idpaterson/gists{/gist_id}","starred_url":"https://api.github.com/users/idpaterson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/idpaterson/subscriptions","organizations_url":"https://api.github.com/users/idpaterson/orgs","repos_url":"https://api.github.com/users/idpaterson/repos","events_url":"https://api.github.com/users/idpaterson/events{/privacy}","received_events_url":"https://api.github.com/users/idpaterson/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"idpaterson","id":507058,"avatar_url":"https://avatars.githubusercontent.com/u/507058?v=3","gravatar_id":"","url":"https://api.github.com/users/idpaterson","html_url":"https://github.com/idpaterson","followers_url":"https://api.github.com/users/idpaterson/followers","following_url":"https://api.github.com/users/idpaterson/following{/other_user}","gists_url":"https://api.github.com/users/idpaterson/gists{/gist_id}","starred_url":"https://api.github.com/users/idpaterson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/idpaterson/subscriptions","organizations_url":"https://api.github.com/users/idpaterson/orgs","repos_url":"https://api.github.com/users/idpaterson/repos","events_url":"https://api.github.com/users/idpaterson/events{/privacy}","received_events_url":"https://api.github.com/users/idpaterson/received_events","type":"User","site_admin":false},"assignees":[{"login":"idpaterson","id":507058,"avatar_url":"https://avatars.githubusercontent.com/u/507058?v=3","gravatar_id":"","url":"https://api.github.com/users/idpaterson","html_url":"https://github.com/idpaterson","followers_url":"https://api.github.com/users/idpaterson/followers","following_url":"https://api.github.com/users/idpaterson/following{/other_user}","gists_url":"https://api.github.com/users/idpaterson/gists{/gist_id}","starred_url":"https://api.github.com/users/idpaterson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/idpaterson/subscriptions","organizations_url":"https://api.github.com/users/idpaterson/orgs","repos_url":"https://api.github.com/users/idpaterson/repos","events_url":"https://api.github.com/users/idpaterson/events{/privacy}","received_events_url":"https://api.github.com/users/idpaterson/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/bear/parsedatetime/milestones/2","html_url":"https://github.com/bear/parsedatetime/milestones/3.0","labels_url":"https://api.github.com/repos/bear/parsedatetime/milestones/2/labels","id":1960370,"number":2,"title":"3.0","description":"","creator":{"login":"idpaterson","id":507058,"avatar_url":"https://avatars.githubusercontent.com/u/507058?v=3","gravatar_id":"","url":"https://api.github.com/users/idpaterson","html_url":"https://github.com/idpaterson","followers_url":"https://api.github.com/users/idpaterson/followers","following_url":"https://api.github.com/users/idpaterson/following{/other_user}","gists_url":"https://api.github.com/users/idpaterson/gists{/gist_id}","starred_url":"https://api.github.com/users/idpaterson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/idpaterson/subscriptions","organizations_url":"https://api.github.com/users/idpaterson/orgs","repos_url":"https://api.github.com/users/idpaterson/repos","events_url":"https://api.github.com/users/idpaterson/events{/privacy}","received_events_url":"https://api.github.com/users/idpaterson/received_events","type":"User","site_admin":false},"open_issues":9,"closed_issues":0,"state":"open","created_at":"2016-08-24T12:21:34Z","updated_at":"2016-09-10T14:54:52Z","due_on":null,"closed_at":null},"comments":3,"created_at":"2016-09-10T14:54:52Z","updated_at":"2016-09-11T01:02:01Z","closed_at":null,"body":"The current approach to unit tests is quite verbose but makes it very easy to show what failed because each assertion contains the literal value that it is testing. I tried something different in [this pull request](https://github.com/bear/parsedatetime/pull/195).\r\n\r\n### Original\r\n\r\nTake the simple date time tests for an example. The test has 82 lines of code to test 35 conditions:\r\n\r\n```python\r\n    def testTimes(self):\r\n        start = datetime.datetime(\r\n            self.yr, self.mth, self.dy, self.hr, self.mn, self.sec).timetuple()\r\n        target = datetime.datetime(\r\n            self.yr, self.mth, self.dy, 23, 0, 0).timetuple()\r\n\r\n        self.assertExpectedResult(\r\n            self.cal.parse('11:00:00 PM', start), (target, 2))\r\n        self.assertExpectedResult(\r\n            self.cal.parse('11:00 PM', start), (target, 2))\r\n        self.assertExpectedResult(\r\n            self.cal.parse('11 PM', start), (target, 2))\r\n        [...]\r\n        self.assertExpectedResult(\r\n            self.cal.parse('\"11 p.m.\"', start), (target, 2))\r\n\r\n        target = datetime.datetime(\r\n            self.yr, self.mth, self.dy, 11, 0, 0).timetuple()\r\n\r\n        self.assertExpectedResult(\r\n            self.cal.parse('11:00:00 AM', start), (target, 2))\r\n        self.assertExpectedResult(\r\n            self.cal.parse('11:00 AM', start), (target, 2))\r\n        [...]\r\n        self.assertExpectedResult(\r\n            self.cal.parse('(11 a.m.)', start), (target, 2))\r\n\r\n        target = datetime.datetime(\r\n            self.yr, self.mth, self.dy, 7, 30, 0).timetuple()\r\n\r\n        self.assertExpectedResult(self.cal.parse('730', start), (target, 2))\r\n        self.assertExpectedResult(self.cal.parse('0730', start), (target, 2))\r\n        self.assertExpectedResult(self.cal.parse('0730am', start), (target, 2))\r\n\r\n        target = datetime.datetime(\r\n            self.yr, self.mth, self.dy, 17, 30, 0).timetuple()\r\n\r\n        self.assertExpectedResult(self.cal.parse('1730', start), (target, 2))\r\n        self.assertExpectedResult(self.cal.parse('173000', start), (target, 2))\r\n```\r\n\r\n### Small improvement\r\n\r\nIn #195 wanted to test all of these conditions in `nlp` as well but converting all of those tests to expect the result from `nlp` (which includes the phrase that matched) just wasn't going to happen. Trying to follow this pattern with such repetitive results for the results from `nlp` was a whole new level of unmaintainability.\r\n\r\nThis approach allows prefixes and other programmatic modifications to all of the test targets for more comprehensive testing. For example, by running everything through the \"wrap in quotes\" test I found that `\"August 25\"` was not parsed.\r\n\r\nThere are now 62 lines of code for 66 tests:\r\n\r\n```python\r\n    def testTimes(self):\r\n        start = datetime.datetime(\r\n            self.yr, self.mth, self.dy, self.hr, self.mn, self.sec).timetuple()\r\n        targets = {\r\n            datetime.datetime(self.yr, self.mth, self.dy, 23, 0, 0): (\r\n                '11:00:00 PM',\r\n                '11:00 PM',\r\n                '11 PM',\r\n                [...]\r\n                '11 p.m.',\r\n            ),\r\n            datetime.datetime(self.yr, self.mth, self.dy, 11, 0, 0): (\r\n                '11:00:00 AM',\r\n                '11:00 AM',\r\n                [...]\r\n                '11 a.m.',\r\n            ),\r\n            datetime.datetime(self.yr, self.mth, self.dy, 7, 30, 0): (\r\n                '730',\r\n                '0730',\r\n                '0730am',\r\n            ),\r\n            datetime.datetime(self.yr, self.mth, self.dy, 17, 30, 0): (\r\n                '1730',\r\n                '173000',\r\n            )\r\n        }\r\n\r\n        for dt, phrases in targets.iteritems():\r\n            # Time (2) phrase starting at index 0\r\n            target = (dt, 2, 0)\r\n            for phrase in phrases:\r\n                self.assertExpectedResult(\r\n                    self.cal.nlp(phrase, start),\r\n                    (target + (len(phrase), phrase),)\r\n                )\r\n\r\n            # Wrap in quotes\r\n            target = (dt, 2, 1)\r\n            for phrase in phrases:\r\n                self.assertExpectedResult(\r\n                    self.cal.nlp('\"%s\"' % phrase, start),\r\n                    (target + (len(phrase) + 1, phrase),)\r\n                )\r\n```\r\n\r\nThis isn't the best structure for tests but at least you can still easily see what failed:\r\n\r\n```python\r\ntests/utils.py:22: in decoratedComparator\r\n    self.fail(failureMessage % (errMsg, result, check))\r\nE   AssertionError: Result does not match target value\r\nE   \r\nE   \tResult:\r\nE   \tNone\r\nE   \r\nE   \tExpected:\r\nE   \t((datetime.datetime(2016, 8, 25, 8, 49, 53), 1, 1, 10, u'August 25'),)\r\n```\r\n\r\n### Share data between tests\r\n\r\nIf we were to store this test data outside of the test case itself we could split these two basic and wrapped in quotes tests into two different test methods. Pull in a fixture, apply modifications, pass it through `nlp` and test that you still get the expected output.\r\n\r\n### Share data between `parse` and `nlp`\r\n\r\nWith a test fixture format like this we could also share test cases between `nlp` and `parse` to ensure that they behave consistently. \r\n\r\npytest has excellent support for creating and loading fixtures. It looks like pytest could offer much cleaner and more reusable tests than unittest does since data could be loaded with decorators rather than forcing a subclass syntax for each group of tests using the same data. I'm not an expert in python unit testing so someone else will probably have more useful input here.\r\n\r\n### Share test cases between locales\r\n\r\nTaking it another step, the fixture data could be translated for each locale. This would allow for much more thorough testing and easier test setup for locales.\r\n\r\nThe tests above could be boiled down to something like this that runs through a full collection of test cases for each locale.\r\n\r\n```python\r\n@pytest.mark.parametrize('cal,sourceTime,phrase,dt', simpleTimesByLocale)\r\ndef test_simple_times_wrapped_in_quotes(cal, sourceTime, phrase, dt):\r\n    mod_phrase = u'\"%s\"' % phrase\r\n    assert cal.nlp(mod_phrase, sourceTime) == (dt, 2, 1, len(phrase) + 1, phrase)\r\n```\r\n\r\nHere we have 4 lines of code for over 200 tests with a very clear test name and single set of arguments that will be logged with a failure. Of course there would be a lot of logic needed to generate the per-locale test data, this may or may not be the way to do it. In some cases it may not be necessary to balloon the number of tests this much, but in other cases it may help prevent regressions and inconsistencies. \r\n\r\nYou will also see each failure rather than the entire test stopping at the first failed input. A max failure setting would avoid showing thousands of failures if the contributor really broke something.\r\n\r\n### Special considerations for `parse`\r\n\r\nWhen the above simple modified test for `nlp` fails, it is easy to see what was being tested because the assertion includes the phrase. However, `parse` does not include the phrase in its return value so looping over values in that way would give no insight into what actually failed. \r\n\r\nParameterized test functions like the one shown above would not have this problem since the arguments are printed with the failure message and each invocation has only a single datapoint to test rather than looping over a list.\r\n\r\n### Data format\r\n\r\nAny suggestions for a good format for this kind of test fixture data? YAML is probably fairly common and might be easier to read and translate than python but it introduces the potential difficulty of trying to structure something that would be easier in python. \r\n\r\nThe format shown above is not portable enough, just something I set up to quickly get these few tests running. The parametrized example above assumes some intermediary processing to flatten the data format down to a tuple of `(calendar, source time, phrase, expected datetime)`, we wouldn't want the test data to have to specify all of those explicitly for every case."},"comment":{"url":"https://api.github.com/repos/bear/parsedatetime/issues/comments/246155082","html_url":"https://github.com/bear/parsedatetime/issues/196#issuecomment-246155082","issue_url":"https://api.github.com/repos/bear/parsedatetime/issues/196","id":246155082,"user":{"login":"bear","id":1267,"avatar_url":"https://avatars.githubusercontent.com/u/1267?v=3","gravatar_id":"","url":"https://api.github.com/users/bear","html_url":"https://github.com/bear","followers_url":"https://api.github.com/users/bear/followers","following_url":"https://api.github.com/users/bear/following{/other_user}","gists_url":"https://api.github.com/users/bear/gists{/gist_id}","starred_url":"https://api.github.com/users/bear/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bear/subscriptions","organizations_url":"https://api.github.com/users/bear/orgs","repos_url":"https://api.github.com/users/bear/repos","events_url":"https://api.github.com/users/bear/events{/privacy}","received_events_url":"https://api.github.com/users/bear/received_events","type":"User","site_admin":false},"created_at":"2016-09-11T01:02:01Z","updated_at":"2016-09-11T01:02:01Z","body":"I like the last example you gave - being able to have the tests specify as the key the phrase and the value the expected result.  Instead of having `dow_parse_style: 1` be a peer of `source_time` and `phrases` I would suggest we add a more explicit section name and then allow anything inside of it to change any `pdc` constant"}}
{"actor":{"display_login":"bear"}}