Skip to content
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

[DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573

Open
8 of 12 tasks
Tracked by #14102
EricGao888 opened this issue Jun 23, 2022 · 20 comments
Open
8 of 12 tasks
Tracked by #14102

[DSIP-10][Unit Tests] Improve DolphinScheduler unit tests #10573

EricGao888 opened this issue Jun 23, 2022 · 20 comments
Assignees
Labels
backend discussion discussion DSIP help wanted Extra attention is needed improvement make more easy to user or prompt friendly
Milestone

Comments

@EricGao888
Copy link
Member

EricGao888 commented Jun 23, 2022

Search before asking

  • I had searched in the issues and found no similar feature requirement.

Description

Unit Tests of high quality could not only improve the stability of Dolphin Scheduler, but also increase the readability of Dolphin Scheduler Code.

  • Currently we only have around 40% UT coverage. Although DS already has E2E tests and the community are working on the API tests, it never means we do not need high quality unit tests.
  • As a DS developer, every time after submitting a PR, we will see the current UT coverage of the whole project. If the coverage is too low, developers may have concerns about the stability of the application.
  • Reading unit tests is one of the best approaches for new developers to understand the code. UT cases also help reviewers to review PR more easily. Furthermore, as it is hardly possible to provide docs of every detailed feature of DS, the best way is to keep UT updated.

Use case

  • We could have different standards for UT for different parts of DS. For example, we have higher bar on UT for core features but lower bar for plugins.
  • We could offer a list of UT cases which need to be updated, added or refactored and label them with good first issue. For new developers to this community, writing UT is a good way to understand the code logic and it is more intriguing than refactoring docs.

Related issues

Action Items

Refactoring Guideline

  • We use method coverage as refactoring metric to decouple UT cases as much as possible.
  • We refactor UTs with jUnit 5 instead of jUnit 4.
  • We should avoid using Powermock in UTs and remove legacy UTs which contains Powermock, see: [Improvement][Test] Remove dependency of powermock #11405 .
  • Scope of each PR:

Progress

  • Arranged by priority:
Module Sub-Module Class issue status
dolphinscheduler-api
dolphinscheduler-master
dolphinscheduler-worker
dolphinscheduler-common
dolphinscheduler-service
dolphinscheduler-alert dolphinscheduler-alert-server AlertServer
dolphinscheduler-task-plugin

Appendix: UT coverage of each module before refactoring

image
image
image
image

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@EricGao888 EricGao888 added feature new feature Waiting for reply Waiting for reply labels Jun 23, 2022
@github-actions
Copy link

Thank you for your feedback, we have received your issue, Please wait patiently for a reply.

  • In order for us to understand your request as soon as possible, please provide detailed information、version or pictures.
  • If you haven't received a reply for a long time, you can join our slack and send your question to channel #troubleshooting

@SbloodyS SbloodyS added backend and removed Waiting for reply Waiting for reply labels Jun 23, 2022
@SbloodyS
Copy link
Member

That's great!

@EricGao888
Copy link
Member Author

That's great!

@SbloodyS Thx. Hope this will help and attract more new developers to submit PRs for DS and offer them an approach to get familiar with the code smoothly.

@davidzollo
Copy link
Contributor

great

@EricGao888
Copy link
Member Author

IMHO, we need some priorities, we could apply high priority to those modules where UT might play a more significant role regarding to code quality and application stability.

@EricGao888
Copy link
Member Author

@SbloodyS Could you please help add a discussion label to this issue? Much appreciated~

@SbloodyS SbloodyS added the discussion discussion label Jun 23, 2022
@SbloodyS
Copy link
Member

IMHO, we need some priorities, we could apply high priority to those modules where UT might play a more significant role regarding to code quality and application stability.

We can make a list and mark it with priority.

@EricGao888
Copy link
Member Author

I got blocked by some unit test when developing a new feature and mocking some unusual stuff for nearly a day. I would like to update the UT docs with some examples which may help new developers bypass or overcome some pitfalls in UTs. May I ask whether it is possible to migrate this docs into main repo? @zhongjiajie https://dolphinscheduler.apache.org/en-us/community/development/unit-test.html

Thanks

@zhongjiajie
Copy link
Member

I got blocked by some unit test when developing a new feature and mocking some unusual stuff for nearly a day. I would like to update the UT docs with some examples which may help new developers bypass or overcome some pitfalls in UTs. May I ask whether it is possible to migrate this docs into main repo? @zhongjiajie https://dolphinscheduler.apache.org/en-us/community/development/unit-test.html

Thanks

Of cause, we can, do you interesting in this migration?

@EricGao888
Copy link
Member Author

I got blocked by some unit test when developing a new feature and mocking some unusual stuff for nearly a day. I would like to update the UT docs with some examples which may help new developers bypass or overcome some pitfalls in UTs. May I ask whether it is possible to migrate this docs into main repo? @zhongjiajie https://dolphinscheduler.apache.org/en-us/community/development/unit-test.html
Thanks

Of cause, we can, do you interesting in this migration?

Yes, I'd love to give a shot.

@EricGao888
Copy link
Member Author

BTW, we could fix the formatting and style errors incrementally, together with unit tests. See: #10963
One convenient way is when refactoring some UTs, we fix the formatting and style errors of both the testing and tested part.

@EricGao888
Copy link
Member Author

I've generated test coverage reports for every module of DS using Intellij, sorted by method coverage. Here are some results:
image
image
image
image

@EricGao888 EricGao888 changed the title [Improvement][Unit Tests] Improve DolphinScheduler unit tests [DSIP-10][Unit Tests] Improve DolphinScheduler unit tests Jul 26, 2022
@EricGao888 EricGao888 added DSIP help wanted Extra attention is needed labels Jul 26, 2022
@EricGao888
Copy link
Member Author

I will update this issue with a more detailed refactoring plan as well as priorities later this week.

@stalary
Copy link
Contributor

stalary commented Sep 26, 2022

I can get involved in the junit upgrade~ @EricGao888

@EricGao888
Copy link
Member Author

I can get involved in the junit upgrade~ @EricGao888

@stalary Currently we are working on removing powermock because it does not support jUnit 5 and blocks the upgrade. Would u like to participate in #11405 ? Thanks~

@stalary
Copy link
Contributor

stalary commented Sep 26, 2022

Currently we are working on removing powermock because it does not support jUnit 5 and blocks the upgrade. Would u like to participate in

Okay, I can be a part of that.

@EricGao888
Copy link
Member Author

Currently we are working on removing powermock because it does not support jUnit 5 and blocks the upgrade. Would u like to participate in

Okay, I can be a part of that.

@stalary Great! We have #12150 open to contributors. Would u like to pick it up? Thanks

@stalary
Copy link
Contributor

stalary commented Sep 26, 2022

Currently we are working on removing powermock because it does not support jUnit 5 and blocks the upgrade. Would u like to participate in

Okay, I can be a part of that.

@stalary Great! We have #12150 open to contributors. Would u like to pick it up? Thanks

Please assign to me, thanks~

@EricGao888
Copy link
Member Author

Usage of Powermock has been fully removed from the whole project : )

@zhongjiajie
Copy link
Member

Usage of Powermock has been fully removed from the whole project :

Great new

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend discussion discussion DSIP help wanted Extra attention is needed improvement make more easy to user or prompt friendly
Projects
Status: Todo
Development

No branches or pull requests

6 participants