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

Add tests configurations #41

Merged
merged 5 commits into from Jun 18, 2021
Merged

Add tests configurations #41

merged 5 commits into from Jun 18, 2021

Conversation

antoinejeannot
Copy link
Member

@antoinejeannot antoinejeannot commented Jun 17, 2021

This PR aims at introducing test_cases in the CONFIGURATIONS map defined at class level, so that to be able to write tests restricted to a given model.

Moreover, TEST_CASES is now the way to go if you want to define upper-level class tests, which will be ran for every model configuration.

E.g.:

class TestableModel(Model[ModelItemType, ModelItemType]):
    CONFIGURATIONS: Dict[str, Dict] = {
        "some_model_a": { 
            "test_cases": {
                "cases": [
                    {"item": {"x": 1}, "result": {"x": 1}},
                    {"item": {"x": 2}, "result": {"x": 2}},
                ],
            }
        },
        "some_model_b": {},
    }
    def _predict(self, item):
        return item

Thanks for reviewing

@victorbenichoux
Copy link
Collaborator

Thank you so much for the contribution, this is extremely useful. I think we need to remove the added level of cases in both of the uses, but other than that it looks good to me

@victorbenichoux
Copy link
Collaborator

victorbenichoux commented Jun 17, 2021

I added a couple of commits, fixing some naming issues, removing the "cases" sublevel, and also adding the correct type for TEST_CASES. PTAL @antoinejeannot

@antoinejeannot
Copy link
Member Author

antoinejeannot commented Jun 18, 2021

I added a couple of commits, fixing some naming issues, removing the "cases" sublevel, and also adding the correct type for TEST_CASES. PTAL @antoinejeannot

Great ! I didn't know if we wanted to keep the extra "cases" level, so I made it so that both systems are consistent.
And you're right, it's better to remove it for now since it is not used at all.
Thanks for the additional commits, I'll check this out

@antoinejeannot
Copy link
Member Author

antoinejeannot commented Jun 18, 2021

Perfect, thanks for your help!

@antoinejeannot antoinejeannot merged commit b21999e into main Jun 18, 2021
@antoinejeannot antoinejeannot deleted the add-tests-configurations branch June 18, 2021 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants