Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datetime tester feeds optional params; need tests with only positional args #195

Open
ChanceNCounter opened this issue May 9, 2021 · 3 comments
Labels
bug Something isn't working

Comments

@ChanceNCounter
Copy link
Contributor

ChanceNCounter commented May 9, 2021

The standardization pass on parse_de.py seems to have messed up certain cases that weren't detected until the update made it all the way to Mycroft-core.

It's the datetime tester. The datetime tester has both the optional params populated in all languages. This inherently misses all nondefault invocations. Glaring oversight. At least seven programmers and four reviewers, including myself.

This ticket could be an adage about impostor syndrome, Dunning-Kruger, and the fictional nature of 100% test coverage.

@ChanceNCounter ChanceNCounter added bug Something isn't working de related to German language good first issue Good for newcomers help wanted Extra attention is needed and removed bug Something isn't working labels May 9, 2021
@ChanceNCounter
Copy link
Contributor Author

For example, extract_datetime('2 wochen') and extract_datetime(2 tage) both fail in the absence of an explicitly-passed anchor date.

These calls passed in LF 0.2.3, and the change didn't make any tests fail. We discovered the problem when a German-speaking Mycroft user tried it.

@ChanceNCounter ChanceNCounter added bug Something isn't working and removed de related to German language good first issue Good for newcomers help wanted Extra attention is needed labels May 9, 2021
@ChanceNCounter ChanceNCounter changed the title More robust unit tests for German datetime extractors Datetime tester feeds optional params; need tests with only positional args May 9, 2021
@emphasize
Copy link
Contributor

emphasize commented May 9, 2021

I don't think this would be caught on any language since the tests always give an anchorDate

date = datetime(2017, 6, 27, 0, 0)
[extractedDate, leftover] = extract_datetime(text, date)

suggestion would be to throw in some tests without the need to set a specific date

eg

heute=lingua_franca.time.now_local()
def test_extractdatetime_de(self):
        def testExtract(text, expected_date, expected_leftover):
            kwargs={"lang":"de-de"}
            if isinstance(expected_date,str):
                kwargs["anchorDate"]=datetime(2017, 6, 27, 0, 0)
            else:
                expected_date=expected_date.replace(hour=0, minute=0, second=0).strftime("%Y-%m-%d %H:%M:%S")

            extractedDate, leftover = extract_datetime(text, **kwargs)
            extractedDate = extractedDate.strftime("%Y-%m-%d %H:%M:%S")

            self.assertEqual(extractedDate, expected_date)
            self.assertEqual(leftover, expected_leftover)

        testExtract("setze den frisörtermin auf 5 tage von heute",
                    heute+timedelta(days=5), "setze frisörtermin")

@ChanceNCounter
Copy link
Contributor Author

Agreed. I think some/most of this still involves those JSON parsers, so it'll be more than a five-minute fix.

Take a stab, if you like; if not, I'll throw it on the roadmap thingy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants