[feat-2676]: e2e testing for runner.RunTask

Open keon94 opened this issue 3 years ago • 2 comments

Summary

Does this close any open issues?

Closes #2676

Screenshots

Other Information

Any other information that is important to this PR.

Aug 03 '22 19:08 keon94

e2e tests currently failing because of this change

Sep 16 '22 04:09 keon94

Hi, Keon, looks like you are writitest casesses for the runner module. Would you move them to the module? I don't think plugins module is the right place to be.

Hey Klesh. I can do that, but since this is an e2e test, not a unit test, it'll be unusual. That's why I added it in under plugins/.

Sep 16 '22 17:09 keon94

I would prefer writing unit-test for runner instead of e2e-test. The problems we ran into in the past were things likes "progress blocking the whole system", and "failed to handle panic". Check the https://github.com/apache/incubator-devlake/commits/main/runner for detail.

It would make more sense for us to cook a set of unit-tests to cover those critical issues we met in the past. These e2e-tests involve too many modules and provide limited guarantees in contrast to the maintenance overhead..

Oct 17 '22 11:10 klesh

Hi, @keon94, I'm cleaning up the PRs, What should we do for this PR? I still don't think e2e-test for runner is a good idea. If you believe this should be merged, feel free to talk to other @hezyin and other dev, we can merge it if most of us agreed.

Nov 23 '22 06:11 klesh

Hi, @keon94, I'm cleaning up the PRs, What should we do for this PR? I still don't think e2e-test for runner is a good idea. If you believe this should be merged, feel free to talk to other @hezyin and other dev, we can merge it if most of us agreed.

I'm ok with discarding and closing this. I have ideas about adding broader integration tests in the test/api package later.

Nov 23 '22 21:11 keon94