feast-dev / feast

The Open Source Feature Store for Machine Learning
https://feast.dev
Apache License 2.0
5.52k stars 987 forks source link

Defining Integration Test and Possible test Improvements #981

Closed mrzzy closed 3 years ago

mrzzy commented 4 years ago

This issue tracks open questions and possible improvements that can be made to Integration tests.

Problem

Currently the integration tests are fuzzy on how things work:

Proposal

Refactor Integration Tests:

Standardization:

Improve Developer Documentation:

woop commented 4 years ago

Many of the questions here are very specific questions on implementation details. I will let @pyalex answer w.r.t the integration test framework that he kicked off, and how we could use it.

My only comments with regards to those tests are

  1. Lets focus on the essential complexity. We want test authors to only define what is different to their environment. Maybe auth is on, maybe there is a rate limiter or some request processor or something.
  2. Developers should use their own judgement when deciding which types of tests to write. Unit test are faster than integration, integration are typically faster than e2e. Whatever is the fastest and most representative way to do a test. If you cant do a unit test without mocking a database, then maybe you should use an integration test with a docker container. If you need a complete Feast deployment, then maybe you need to use Docker Compose, etc.
  3. Lets be aggressive in deduplicating infra provisioning code. Right now the infra/scripts folder contains a lot of test code to set up infra, and this is being duplicated to some degree with Java based integration tests. Ideally we would have a single way to provision and seed both a docker compose and a single way to provision a Kubernetes/helm based environment. Ideally the test suites should only kick off the setup/teardown of one of these two with the non-standard config applied, and the tests themselves should not even be aware of what provisioned the Feast environment.

What constitutes an integration test?

Are we saying our definition of an integration test is non-standard from the industry?

How much mocking is allowed in an integration test?

You can mock as much as you want, but it's a poor test if you mock too much. Once you start integrating multiple components together you might as well provision the resources behind them. Integration tests do not preclude mocking.

Use the commit hash to pull docker image built for the current code.

With integration tests, normally you would not rebuild dependent services on each commit. For example if you are working on Feast Serving you would build that jar on every new commit or change, but Feast Core can just use your latest container image. Otherwise it takes 10+ minutes to test each change.

pyalex commented 4 years ago

How to start up dependencies:

I prefer docker (via testcontainers) as main approach for all dependencies. Not necessarily docker-compose, since it's much more complicated and has issue with ports. But docker is universal solution, and if we, say, decide to get rid of spring in core or write it in other language - that would be a safer solution. In general modules should not care about other implementations. Given example with starting spring application in jobcontroller was a mistake. I had changes in both core & job controller in the same PR, but better approach would be to change API in core first.

should have my own infra setup for each test? (ie separate docker compose files or use a common docker-compose file?) how do we keep all this infra in sync if separate?

right now we have dedicated module common-test where we can collect all needed configurations and reuse it, no need to reinvent it every time. The same applies to CoreSimpleAPIClient or SimpleCoreClient example. It was simply created before there was this module. So that's just refactoring issue now.

subclass BaseIT or BaseAuthIT ?

BaseAuthIT again was created for serving, when there was no common-test and it differs in default environment. BaseIT provides postgres & kafka, whereas BaseAuthIT assumes there's redis. There's definitely naming issue and refactoring needed.

How much mocking is allowed in an integration test?

In IT you cannot mock staff, since you're dealing with almost a black box. However you can replace some implementations. Because, unfortunately, you can't spawn bigquery in docker. FakeJobManager is not a mock, but rather minimally functional implementation of JobManager interface, that keeps some state in memory and tries to give consistent API. I think such things are totally ok, when you can't dockerize 3rd party service.

In general current integration test system is not complete and far from ideal. My idea was to build it from down to top. First cover some specific cases and then abstract & generalize. We should be moving in this direction with each PR, since it's impossible to predict all use cases in advance.

pyalex commented 4 years ago

Summarising: refactoring & standartization are totally needed. No doubts.

terryyylim commented 4 years ago

In addition, we may also want to consider adding tests to ensure these helm tests are actually working as expected.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.