-
Notifications
You must be signed in to change notification settings - Fork 224
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
OPIK-645 Return all feedback score names for project ids endpoint #947
base: main
Are you sure you want to change the base?
OPIK-645 Return all feedback score names for project ids endpoint #947
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is not placed in the right resource test class, to me that's a blocker.
The rest are minor or non-blocking comments.
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ProjectsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/ProjectsResource.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/FeedbackScoreDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/FeedbackScoreDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/FeedbackScoreDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/FeedbackScoreDAO.java
Show resolved
Hide resolved
...ackend/src/test/java/com/comet/opik/api/resources/utils/resources/ProjectResourceClient.java
Outdated
Show resolved
Hide resolved
var feedbackScoreNamesByProjectId = projectResourceClient.findFeedbackScoreNames(projectIdsQueryParam, apiKey, workspaceName); | ||
assertFeedbackScoreNames(feedbackScoreNamesByProjectId, expectedNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a new test for project resource, not under the experiments resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to ProjectsResourceTest
and did some refactoring.
cfd71c8
to
888ed03
Compare
Details
Return all feedback score names for project ids endpoint
Testing
Covered by integration test