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

chore: Make feature server test a unit test #4694

Merged
merged 12 commits into from
Oct 25, 2024

Conversation

robhowley
Copy link
Contributor

@robhowley robhowley commented Oct 25, 2024

What this PR does / why we need it:

  • the feature server integration tests are only doing integration testing that's already covered by test_universal_online, i.e. feature_store -> data_store
  • the code inside the path operation can be tested by unit tests
  • this should also avoid some of the event loop flakiness arising from
    • fastapi test client, not actual server isolation
    • context management w a shared store
    • 3rd party services

Which issue(s) this PR fixes:

Misc

Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
@robhowley robhowley changed the title chore: make feature server test a unit test chore: Make feature server test a unit test Oct 25, 2024
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
Signed-off-by: Rob Howley <howley.robert@gmail.com>
@robhowley robhowley marked this pull request as ready for review October 25, 2024 18:05
@robhowley robhowley requested a review from a team as a code owner October 25, 2024 18:05
Signed-off-by: Rob Howley <howley.robert@gmail.com>


@pytest.mark.integration
@pytest.mark.universal_online_stores

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This runs the tests for all online stores and basically tests the feature server end to end.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does, but using the fake server via fastapi testclient so not a "true" integration e2e. what ends up happening is that it's basically a unit test of the server and integration test for the feature store. the integration test of the feature store w the diff online stores already happens in test_universal_online. there isn't, however, a test suite dedicated to working w the server, only that implicit one baked into this file.

since this construct is proving to be complicated and brittle, seemed much simpler to break the implicit unit test out from the integration test and let the two exist on their own. imo if we really wanted a full e2e we'd start up a server and make calls to it w a regular client, not the fastapi one. reasonable people can def disagree on this file, but figured worth considering this option since integration tests have been failing inconsistently.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I can get behind that.

@franciscojavierarceo franciscojavierarceo merged commit 658b18f into feast-dev:master Oct 25, 2024
25 checks passed
@robhowley robhowley deleted the rh-unit-test branch October 25, 2024 20:22
lokeshrangineni pushed a commit to lokeshrangineni/feast that referenced this pull request Oct 29, 2024
* make feature server tests unit tests, remove integration

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* use local example repo for test

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* use example repo 1, switch to drive loc

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* missing timestamp in payload

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* combine tests

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* simplify the request bodies

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* use diff feature view

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* fix feature name

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* approx equal

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* approx equal

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* fix response format assertion

Signed-off-by: Rob Howley <howley.robert@gmail.com>

* dont need lazy fixture, simplify

Signed-off-by: Rob Howley <howley.robert@gmail.com>

---------

Signed-off-by: Rob Howley <howley.robert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants