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

Revisit tests #2811

Closed
masci opened this issue Jul 14, 2022 · 2 comments
Closed

Revisit tests #2811

masci opened this issue Jul 14, 2022 · 2 comments
Assignees

Comments

@masci
Copy link
Contributor

masci commented Jul 14, 2022

Problem

At the moment there is some confusion about how test functions are categorized and it's not easy to distinguish between unit and integration tests. Fixing this problem would improve the development experience by making the tests fail fast if there is an issue and by making it more obvious what's the issue.

Proposal

We formally define three scopes for tests in Haystack with different requirements and purposes; newly added tests will follow those requirements while we progressively adapt the existing tests to the new model.

The three scopes are defined as follows:

  • Unit test
    • Tests a single logical concept
    • Execution time is a few milliseconds
    • Any external resource is mocked
    • Always returns the same result
    • Can run in any order
    • Runs at every commit in draft and ready PRs, automated through pytest
    • Can run locally with no additional setup
    • Goal: being confident in merging code
  • Integration test
    • Tests a single logical concept
    • Execution time is a few seconds
    • It uses external resources that must be available before execution
    • When using models, cannot use inference
    • Always returns the same result or an error
    • Can run in any order
    • Runs at every commit in ready PRs, automated through pytest
    • Can run locally with some additional setup (e.g. Docker)
    • Goal: being confident in merging code
  • End to End (e2e) test
    • Tests a sequence of multiple logical concepts
    • Execution time has no limits (can be always on)
    • Can use inference
    • Evaluates the results of the execution or the status of the system
    • It uses external resources that must be available before execution
    • Can return different results
    • Can be dependent on the order
    • Can be wrapped into any process execution
    • Runs outside the development cycle (nightly or on demand)
    • Might not be possible to run locally due to system and hardware requirements
    • Goal: being confident in releasing Haystack

Action plan

Note: this planning will be subject to heavy changes as we progress understanding how the tests could be improved.

@masci masci added the epic label Jul 14, 2022
@ZanSara ZanSara changed the title Revisit unit and integration tests Revisit tests Jul 27, 2022
@masci masci added the epic:idle Epic not yet started label Jul 28, 2022
@masci masci added epic:in-progress Epic is in progress and removed epic:idle Epic not yet started labels Aug 3, 2022
@ZanSara
Copy link
Contributor

ZanSara commented Aug 3, 2022

Interesting case of tests on the line between integration and end-to-end: #2903

These tests are e2e according to the definition above, because they initialize a model and do inference on it. However, the nodes are classifiers, so for the purpose of testing, mocking the models is trivial and matter of a simple if-else. So, are these tests better seen as end-to-end due to the inference step, or they should rather be mocked and used as integration tests?

Note that these, in my opinion, are too high level to be considered unit tests, regardless of the presence of the mock or their speed. They are testing most of the node at once and imho this is too wide of a scope.

masci added a commit that referenced this issue Sep 5, 2022
Add the outcome of #2811 to the developers docs

Ideally, newly added tests will follow those requirements while we progressively adapt the existing tests to the new model.
ZanSara pushed a commit that referenced this issue Sep 5, 2022
* Update CONTRIBUTING.md

Add the outcome of #2811 to the developers docs

Ideally, newly added tests will follow those requirements while we progressively adapt the existing tests to the new model.

* address review comments
@masci
Copy link
Contributor Author

masci commented Sep 5, 2022

We decided to split this epic and continue the work in chunks, see the updated issue description. Further conversations will happen in their respective epic.

With the newly reduced scope, I'm going to call this one done and close.

@masci masci closed this as completed Sep 5, 2022
@masci masci added epic:done and removed epic:in-progress Epic is in progress labels Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

2 participants