fix: Refactor testing and sort out unit and integration tests #2975

kevjumba · 2022-07-27T20:51:25Z

What this PR does / why we need it:
Things I did

Refactor unit tests and mark integration tests with the pytest marker
Move all in-file functions/fixtures to the bottom(there were files with functions at top or bottom so I just chose one for consistency) this also allows devs to see the tests as soon as they open the file for clarity)
Refactor out some of the functions used in multiple files to utility classes to make test files cleaner.
Left fixtures in file because of the way pytest works(fixtures can go either in conftest or the specific pytest file where it's used.
Which issue(s) this PR fixes:

Fixes #

felixwang9817

some initial comments

sdk/python/tests/utils/feature_test_utils.py

felixwang9817 · 2022-07-27T21:07:54Z

sdk/python/tests/utils/test_wrapper_utils.py

@@ -0,0 +1,14 @@
+import pytest


[not for this file] I think you can delete online_write_benchmark.py? doesn't seem like it's being used anywhere

yeah i was wondering about that, i know it's not being used but it had a script, maybe @achals and @adchia can weigh in if this is actually needed

felixwang9817

some more comments

sdk/python/tests/utils/e2e_test_utils.py

sdk/python/tests/unit/infra/test_inference_unit_tests.py

felixwang9817 · 2022-07-27T21:58:41Z

sdk/python/tests/unit/infra/online_store/test_e2e_local.py

@@ -217,3 +88,35 @@ def test_e2e_local() -> None:

            assert returncode != 0
            assert "feast.errors.FeastJoinKeysDuringMaterialization" in str(output)
+
+
+def _test_materialize_and_online_retrieval(


seems strange to me that this would be considered a unit test, whereas something like e.g. test_cli_chdir.py is considered integration test

I think we should make this an integration test

honestly I wasn't too sure about this one but I think it makes sense to keep it in integration so I'll move it back and tag.

I left a comment but I think only tests that need a cloud service should be integration tests (mainly cause then we'd be able to run more tests without pre-configuration of credentials etc and get faster coverage.)

sdk/python/tests/utils/feature_test_utils.py

codecov-commenter · 2022-07-27T22:18:29Z

Codecov Report

Merging #2975 (f2696e0) into master (f2696e0) will not change coverage.
The diff coverage is n/a.

❗ Current head f2696e0 differs from pull request most recent head 96b7efb. Consider uploading reports for the commit 96b7efb to get more accurate results

@@           Coverage Diff           @@
##           master    #2975   +/-   ##
=======================================
  Coverage   77.69%   77.69%           
=======================================
  Files         186      186           
  Lines       16387    16387           
=======================================
  Hits        12732    12732           
  Misses       3655     3655

Flag	Coverage Δ
integrationtests	`68.78% <0.00%> (ø)`
unittests	`58.47% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2696e0...96b7efb. Read the comment docs.

achals · 2022-07-27T20:52:58Z

sdk/python/tests/conftest.py

@@ -45,6 +43,7 @@
 from tests.integration.feature_repos.universal.data_sources.file import (  # noqa: E402
    FileDataSourceCreator,
 )
+from tests.utils.http_utils import check_port_open, free_port  # noqa: E402


nit; better names than utils. utils usually end up becoming dumping grounds.

achals · 2022-07-27T20:54:19Z

sdk/python/tests/integration/e2e/test_go_feature_server.py

@@ -11,7 +9,7 @@
 import pytz
 import requests

-from feast import FeatureService, FeatureView, ValueType
+from feast import FeatureService, ValueType


nit, for tests we should be doing from feast.feautre_service import FeatureService instead of from feast import FeatureService since the latter often leads to circular imports.

achals · 2022-07-28T16:39:58Z

sdk/python/tests/integration/registration/test_cli_apply_duplicates.py

+@pytest.mark.integration
 def test_cli_apply_duplicate_data_source_names() -> None:
    run_simple_apply_test(
        example_repo_file_name="example_repo_duplicate_data_source_names.py",


I'll actually argue that a test that can run without any network connectivity (cause it uses file source and sqlite online store) should be a unit test.

Unit tests are more reliable and run faster, and we wanna make more tests unit tests anyway

Applies to the other test_cli_ tests too.

Did major refactor for those

achals · 2022-07-28T16:41:52Z

sdk/python/tests/unit/infra/test_inference_unit_tests.py

+from feast import (
+    BigQuerySource,
+    Entity,
+    Feature,
+    FeatureService,
+    FileSource,
+    RedshiftSource,
+    RepoConfig,
+    SnowflakeSource,
+    ValueType,
+)


Same import comment

achals · 2022-07-28T16:42:09Z

sdk/python/tests/unit/infra/test_local_registry.py

@@ -0,0 +1,535 @@
+# Copyright 2021 The Feast Authors


achals · 2022-07-28T16:42:32Z

sdk/python/tests/unit/test_feature_service.py

@@ -5,6 +5,7 @@
 from feast.field import Field
 from feast.infra.offline_stores.file_source import FileSource
 from feast.types import Float32
+from tests.utils.test_wrapper_utils import no_warnings


too many utils

I renamed it since it doesn't really have to be a utils but I still think having a file with test_wrappers allows code reusability

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

achals

/lgtm

feast-ci-bot · 2022-07-29T20:03:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, kevjumba

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [achals,kevjumba]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

# [0.23.0](v0.22.0...v0.23.0) (2022-08-02) ### Bug Fixes * Add dummy alias to pull_all_from_table_or_query ([#2956](#2956)) ([5e45228](5e45228)) * Bump version of Guava to mitigate cve ([#2896](#2896)) ([51df8be](51df8be)) * Change numpy version on setup.py and upgrade it to resolve dependabot warning ([#2887](#2887)) ([80ea7a9](80ea7a9)) * Change the feature store plan method to public modifier ([#2904](#2904)) ([0ec7d1a](0ec7d1a)) * Deprecate 3.7 wheels and fix verification workflow ([#2934](#2934)) ([040c910](040c910)) * Do not allow same column to be reused in data sources ([#2965](#2965)) ([661c053](661c053)) * Fix build wheels workflow to install apache-arrow correctly ([#2932](#2932)) ([bdeb4ae](bdeb4ae)) * Fix file offline store logic for feature views without ttl ([#2971](#2971)) ([26f6b69](26f6b69)) * Fix grpc and update protobuf ([#2894](#2894)) ([86e9efd](86e9efd)) * Fix night ci syntax error and update readme ([#2935](#2935)) ([b917540](b917540)) * Fix nightly ci again ([#2939](#2939)) ([1603c9e](1603c9e)) * Fix the go build and use CgoArrowAllocator to prevent incorrect garbage collection ([#2919](#2919)) ([130746e](130746e)) * Fix typo in CONTRIBUTING.md ([#2955](#2955)) ([8534f69](8534f69)) * Fixing broken links to feast documentation on java readme and contribution ([#2892](#2892)) ([d044588](d044588)) * Fixing Spark min / max entity df event timestamps range return order ([#2735](#2735)) ([ac55ce2](ac55ce2)) * Move gcp back to 1.47.0 since grpcio-tools 1.48.0 got yanked from pypi ([#2990](#2990)) ([fc447eb](fc447eb)) * Refactor testing and sort out unit and integration tests ([#2975](#2975)) ([2680f7b](2680f7b)) * Remove hard-coded integration test setup for AWS & GCP ([#2970](#2970)) ([e4507ac](e4507ac)) * Resolve small typo in README file ([#2930](#2930)) ([16ae902](16ae902)) * Revert "feat: Add snowflake online store ([#2902](#2902))" ([#2909](#2909)) ([38fd001](38fd001)) * Snowflake_online_read fix ([#2988](#2988)) ([651ce34](651ce34)) * Spark source support table with pattern "db.table" ([#2606](#2606)) ([3ce5139](3ce5139)), closes [#2605](#2605) * Switch mysql log string to use regex ([#2976](#2976)) ([5edf4b0](5edf4b0)) * Update gopy to point to fork to resolve github annotation errors. ([#2940](#2940)) ([ba2dcf1](ba2dcf1)) * Version entity serialization mechanism and fix issue with int64 vals ([#2944](#2944)) ([d0d27a3](d0d27a3)) ### Features * Add an experimental lambda-based materialization engine ([#2923](#2923)) ([6f79069](6f79069)) * Add column reordering to `write_to_offline_store` ([#2876](#2876)) ([8abc2ef](8abc2ef)) * Add custom JSON table tab w/ formatting ([#2851](#2851)) ([0159f38](0159f38)) * Add CustomSourceOptions to SavedDatasetStorage ([#2958](#2958)) ([23c09c8](23c09c8)) * Add Go option to `feast serve` command ([#2966](#2966)) ([a36a695](a36a695)) * Add interfaces for batch materialization engine ([#2901](#2901)) ([38b28ca](38b28ca)) * Add pages for individual Features to the Feast UI ([#2850](#2850)) ([9b97fca](9b97fca)) * Add snowflake online store ([#2902](#2902)) ([f758f9e](f758f9e)), closes [#2903](#2903) * Add Snowflake online store (again) ([#2922](#2922)) ([2ef71fc](2ef71fc)), closes [#2903](#2903) * Add to_remote_storage method to RetrievalJob ([#2916](#2916)) ([109ee9c](109ee9c)) * Support retrieval from multiple feature views with different join keys ([#2835](#2835)) ([056cfa1](056cfa1))

feast-ci-bot added approved size/XXL labels Jul 27, 2022

kevjumba added the ok-to-test label Jul 27, 2022

felixwang9817 reviewed Jul 27, 2022

View reviewed changes

kevjumba force-pushed the testing_infra_docs branch from 72f0c66 to 83d62fe Compare July 27, 2022 22:10

felixwang9817 reviewed Jul 27, 2022

View reviewed changes

sdk/python/tests/utils/feature_test_utils.py Outdated Show resolved Hide resolved

sdk/python/tests/utils/feature_test_utils.py Outdated Show resolved Hide resolved

achals reviewed Jul 28, 2022

View reviewed changes

kevjumba added 17 commits July 29, 2022 11:19

Refactor go feature server

c16d9ad

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

562dcb4

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix e2e tests

d9ab146

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

209d15b

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Verify tests

af96f09

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

374d0af

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Address review

826524b

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

11a059c

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Address review

56c04f0

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

7b6ee9f

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

4835499

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

address review

44164f8

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

375d65d

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

0d49d71

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

ef2465f

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

5b2edb7

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

5397529

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

kevjumba added 13 commits July 29, 2022 11:19

Fix

1e633c0

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

513f95c

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

eaae4f2

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Refactor

5e8734e

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

da8e4fa

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fx lit

43b3900

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

d5aad96

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

update fix

1cb34ed

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

aa6fa79

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Revert

27ef453

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix

1b303ac

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

fix

9067a45

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

Fix lint

96b7efb

Signed-off-by: Kevin Zhang <kzhang@tecton.ai>

kevjumba force-pushed the testing_infra_docs branch from 9cf7507 to 96b7efb Compare July 29, 2022 18:23

achals approved these changes Jul 29, 2022

View reviewed changes

feast-ci-bot assigned achals Jul 29, 2022

feast-ci-bot added the lgtm label Jul 29, 2022

kevjumba added ok-to-test and removed ok-to-test labels Jul 29, 2022

feast-ci-bot merged commit 2680f7b into feast-dev:master Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Refactor testing and sort out unit and integration tests #2975

fix: Refactor testing and sort out unit and integration tests #2975

kevjumba commented Jul 27, 2022 •

edited

Loading

felixwang9817 left a comment

felixwang9817 Jul 27, 2022

kevjumba Jul 27, 2022

felixwang9817 left a comment

felixwang9817 Jul 27, 2022 •

edited

Loading

kevjumba Jul 27, 2022

achals Jul 28, 2022

kevjumba Jul 28, 2022

codecov-commenter commented Jul 27, 2022 •

edited

Loading

achals Jul 27, 2022

kevjumba Jul 28, 2022

achals Jul 27, 2022

kevjumba Jul 28, 2022

achals Jul 28, 2022

achals Jul 28, 2022

kevjumba Jul 28, 2022

achals Jul 28, 2022

kevjumba Jul 28, 2022

achals Jul 28, 2022

kevjumba Jul 28, 2022

achals Jul 28, 2022

kevjumba Jul 28, 2022

achals left a comment

feast-ci-bot commented Jul 29, 2022

fix: Refactor testing and sort out unit and integration tests #2975

fix: Refactor testing and sort out unit and integration tests #2975

Conversation

kevjumba commented Jul 27, 2022 • edited Loading

felixwang9817 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixwang9817 left a comment

Choose a reason for hiding this comment

felixwang9817 Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 27, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achals left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Jul 29, 2022

kevjumba commented Jul 27, 2022 •

edited

Loading

felixwang9817 Jul 27, 2022 •

edited

Loading

codecov-commenter commented Jul 27, 2022 •

edited

Loading