Fix pytest_collection_modifyitems to select benchmark tests only

[achals]

Signed-off-by: Achal Shah [email protected]

What this PR does / why we need it:
Fixes a bug. Additionally, remove test items instead of skipping them. This makes --collect-only more useful, since that flag doesn't filter out skipped flags.
Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

none

[codecov-commenter]

Codecov Report

Merging #1874 (582087d) into master (c6bcb46) will decrease coverage by 22.21%.
The diff coverage is 58.82%.

@@             Coverage Diff             @@
##           master    #1874       +/-   ##
===========================================
- Coverage   84.58%   62.37%   -22.22%     
===========================================
  Files          93       93               
  Lines        7273     7282        +9     
===========================================
- Hits         6152     4542     -1610     
- Misses       1121     2740     +1619     

Flag	Coverage Δ
integrationtests	?
unittests	62.37% <58.82%> (-0.05%)	⬇️

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

Impacted Files	Coverage Δ
sdk/python/tests/conftest.py	71.87% <58.82%> (-28.13%)	⬇️
.../integration/online_store/test_universal_online.py	14.73% <0.00%> (-85.27%)	⬇️
.../integration/online_store/test_online_retrieval.py	17.39% <0.00%> (-82.61%)	⬇️
sdk/python/tests/utils/online_read_write_test.py	18.18% <0.00%> (-81.82%)	⬇️
...fline_store/test_universal_historical_retrieval.py	18.69% <0.00%> (-80.49%)	⬇️
...gration/registration/test_feature_service_apply.py	31.25% <0.00%> (-68.75%)	⬇️
...istration/test_universal_odfv_feature_inference.py	35.71% <0.00%> (-64.29%)	⬇️
sdk/python/feast/infra/online_stores/datastore.py	30.32% <0.00%> (-63.12%)	⬇️
sdk/python/feast/infra/online_stores/redis.py	30.35% <0.00%> (-62.50%)	⬇️
sdk/python/tests/data/data_creator.py	38.09% <0.00%> (-61.91%)	⬇️
... and 43 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 c6bcb46...582087d. Read the comment docs.

[adchia]

Seems like these are exclusive?

If the user does should_run_integration and should_run_benchmark, it ends up only running benchmark tests.

I'd expect this more to be

if not should_run_integration, remove integration tests
if not should_run_benchmarks, remove benchmarks
done

[achals]

I changed the current behaviour to be exclusive.

if user specifies --benchmark and --integration, it runs tests marked with both. If the user specified only one of them, it runs tests with only those marks.

i prefer this behaviour since otherwise we run unit tests during all our integration tests - which seems like a duplication and a waste. It also is more straightforward to reason about, IMO.

[achals]

The reason why integration and benchmark tests are orthogonal is because it's possible for an integration test to need access to resources on aws/gcp, which requires the integration mark so that it can be set up correctly.

[felixwang9817]

/lgtm

[feast-ci-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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,felixwang9817]

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