Skip to content

Fix unit tests not running in feast.core package.#883

Merged
feast-ci-bot merged 6 commits intofeast-dev:masterfrom
mrzzy:fix-feast-core-test
Jul 17, 2020
Merged

Fix unit tests not running in feast.core package.#883
feast-ci-bot merged 6 commits intofeast-dev:masterfrom
mrzzy:fix-feast-core-test

Conversation

@mrzzy
Copy link
Copy Markdown
Collaborator

@mrzzy mrzzy commented Jul 15, 2020 •
edited
Loading

What this PR does / why we need it:
Fixes most feast.core unit tests not running as described in issue #882

  • Reverts JUnit 5 dependency in core/pom.xml
  • Fix any broken unit tests.

Which issue(s) this PR fixes:

Fixes #882

Does this PR introduce a user-facing change?:

NONE

@mrzzy mrzzy requested a review from zhilingc as a code owner July 15, 2020 09:38
@mrzzy mrzzy added the kind/bug label Jul 15, 2020
@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 15, 2020

/ok-to-test

mrzzy
mrzzy commented Jul 15, 2020
View reviewed changes
core/src/main/java/feast/core/service/AccessManagementService.java Outdated
Copy link
Copy Markdown
Collaborator Author

@mrzzy mrzzy Jul 15, 2020

Choose a reason for hiding this comment

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

@dr3s is it possible to remove the ObjectProvider here to remove the code duplication in two nearly identical AccessManagementService() constructors? This makes the code really error prone.
(:disappointed_relieved: I was stuck debugging because I didn't realize there were two constructors.)
It seems that one constructor is used by Spring and one used by tests.

woop
woop reviewed Jul 15, 2020
View reviewed changes
woop
woop reviewed Jul 15, 2020
View reviewed changes
@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 15, 2020

/test test-end-to-end-batch-dataflow

3 similar comments
@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 15, 2020

/test test-end-to-end-batch-dataflow

@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 15, 2020

/test test-end-to-end-batch-dataflow

@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 16, 2020

/test test-end-to-end-batch-dataflow

@feast-ci-bot
Copy link
Copy Markdown
Collaborator

feast-ci-bot commented Jul 16, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrzzy, woop

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@woop
Copy link
Copy Markdown
Member

woop commented Jul 16, 2020

/lgtm

@feast-ci-bot feast-ci-bot removed the lgtm label Jul 17, 2020
@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 17, 2020

/test test-end-to-end-batch

1 similar comment
@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Jul 17, 2020

/test test-end-to-end-batch

@woop
Copy link
Copy Markdown
Member

woop commented Jul 17, 2020

/test test-end-to-end-batch

@woop
Copy link
Copy Markdown
Member

woop commented Jul 17, 2020

/lgtm

@feast-ci-bot feast-ci-bot merged commit 09916dd into feast-dev:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@woop woop woop approved these changes

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees

@woop woop

Labels

approved kind/bug lgtm ok-to-test size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Most Unit Tests are not Running on feast.core package.

3 participants

@mrzzy @feast-ci-bot @woop