Skip to content
This repository has been archived by the owner on Jul 10, 2024. It is now read-only.

SUBMARINE-584. Add unit test for ExperimentRestApi.java #375

Closed
wants to merge 13 commits into from

Conversation

aeioulisa
Copy link
Member

@aeioulisa aeioulisa commented Aug 12, 2020

What is this PR for?

Add unit test for ExperimentRestApi.java

What type of PR is it?

[Improvement]

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/SUBMARINE-584

How should this be tested?

https://travis-ci.org/github/apache/submarine/jobs/717575609

Screenshots (if appropriate)

Questions:

  • Does the licenses files need update? No
  • Is there breaking changes for older versions? No
  • Does this needs documentation? No

@cxorm cxorm requested a review from pingsutw August 12, 2020 08:30
@cxorm
Copy link
Member

cxorm commented Aug 12, 2020

Thanks @aeioulisa for the work.

Could you please change the title of the PR ?
(Please change PR title Submarine 584. xxx to SUBMARINE-584. xxx.)

@aeioulisa aeioulisa changed the title Submarine 584. Add unit test for ExperimentRestApi.java SUBMARINE 584. Add unit test for ExperimentRestApi.java Aug 12, 2020
@aeioulisa aeioulisa changed the title SUBMARINE 584. Add unit test for ExperimentRestApi.java SUBMARINE-584. Add unit test for ExperimentRestApi.java Aug 12, 2020
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

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

LGTM
@aeioulisa Thank you contribution! :-)

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Thanks @aeioulisa for the contribution.
some comments in the below.

@aeioulisa
Copy link
Member Author

Thanks @cxorm @jiwq @pingsutw @liuxunorg for the review and comments.
I had resolved all comments, could you help me review it again?

@jiwq
Copy link
Member

jiwq commented Aug 15, 2020

+1, I recommend renaming the compareExperiment method name to verifyResult and the experiment param can be renamed to actualExperiment, because the method focus to verify the actual and expect, so that we can understand easily. @aeioulisa

@asfgit asfgit closed this in 7ed7cf0 Aug 17, 2020
@jiwq
Copy link
Member

jiwq commented Aug 17, 2020

Thanks @aeioulisa update the last commit. Thanks @cxorm @liuxunorg and @pingsutw help to review.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants