search

DaGeRe / peass

Tool for Performance analysis of software system
GNU Affero General Public License v3.0
10 stars 9 forks source link

Copy files required by KoPeMe to emulator #115

Closed metint closed 1 year ago

metint commented 1 year ago

Fixes https://github.com/jenkinsci/peass-ci-plugin/issues/170

DaGeRe commented 1 year ago

Thanks for the PR, looks overall fine to me. Would it technically be possible to create a unit test for this implementation? As far as I see it, one would need an Anbox environment (and this kind of integration tests are, from my perspective, to hard to set up for a regular Github Actions build).

metint commented 1 year ago

Before the PR, I thought about the possibility of corresponding unit/integration tests. Although it should be possible in theory, as you mentioned it, it seems quite difficult in practice.

One relatively easy solution might be to run the Anbox emulator in a separate server and redirect the requests to it.

But I suggest reevaluating (and possibly implementing) Android related tests that require Anbox once we complete all required modifications to run emulated tests, if you agree.

DaGeRe commented 1 year ago

One other option might be to move the concrete ProcessBuilder to a separate class (e.g. AdbConnector) and to test whether files get organized correctly and correct methods of the AdbConnector are called. Thereby, we could have a unit test instead of an integration test. However, this is not highly necessary, therefore I'll just merge it.

In general, I am trying to have smaller methods in Peass (I'm not always succeeding here, but try to have it as often as necessary). Therefore, after the merge, please not that I'll do some minor refactorings.

DaGeRe commented 1 year ago

I now did some minor refactoring: f66662fc683403fc151631359e0aeb00ee769e97 and ec7c1e3bf8d413223d7e6d946bac9e364ee7fe1c.

Does it make sense to have filesToBePushed as local variable, or would it make more sense to have it as final field of the class (since it is always the same)?

metint commented 1 year ago

Makes sense, thank you. Maybe in https://github.com/DaGeRe/peass/commit/ec7c1e3bf8d413223d7e6d946bac9e364ee7fe1c you could push creation of File f into the pushSingleFile?

Regarding filesToBePushed I have no clear preference but maybe we can make it a private static final field of the class to make it more visible.