fix: Cleanup old web-ext run artifacts dir when we are connecting to an android device

[Vishalghyv]

(Fixes #1591)

[Vishalghyv]

@rpl Sir the main thing that I have done is adbClient.readdir(deviceId, '/sdcard') and then for their result checked if there is directory and if it then its starting is "web-ext-artifacts-" if so then delete it.
Still checkartifact parameter as mentioned by @Rob--W in #1591 (comment)
"#1591 (comment)"
is not done at all, I will try to figure it out if the current logic could work.

I couldn't get git to commit in the previous one but in this one I have used branch, so probably future commits won't require new pull request.

[rpl]

Hi @Vishalghyv
follows another round of review comments.

Besides the review comments below, I wanted to add some other more general ones:

• are you testing this interactively on an android device (or an android emulator)? (I haven't yet tried it this branch, but in the meantime I want to be sure that you are already able to try your branch while you are designing your proposed fix, let me know if you are unable to run this locally and you would need some help with that)

• it looks that you would like to add an CLI option, take a look to one some of the existing CLI option related to the firefox-android extension runner are defined and how they are getting passed to the firefox-android extension runner instance. Let me know if there are some details about this that you are not yet able to follow.

• As described in the review comments below, from the current version of your patches I got a rough idea of the strategy that you may be thinking of, but your implementation doesn't yet match what I think you want to achieve. In general I would suggest you to decide how the "fix/feature" would work for the user first, then decide how to split and assign the responsibilities to the components involved. It is easier to make clean changes when you have a clear plan.

• I haven't yet commented on the automated tests required for this change, if you don't have some previous experiences on writing unit tests the following comment may provide some useful hints and links to docs: fix: Choose a random free tcp port to be used for the Firefox Desktop remote debugging server  #1669 (comment)

[Vishalghyv]

@rpl , Thanks for reviewing the pull request and giving such brief and informative comments.
I have gone through them and will certainly work on better solution since, the basic logic required seems to be working, but still CLI and Automated tests are left in the next commits these will follow with completely testing the bug :)

[Vishalghyv]

Hey, @rpl I have tried to implement the changes you requested.
Details of the implementation:
checkOrCleanArtifacts is the main function which creates array of directories starting with web-ext-artifacts which is send to remover function which removes the directories.
And this all is controlled by a check boolean type variable which depends on --adb-clean-artifacts cli option
In first round of tests (just to make sure moving in right direction) first remover is tested with two directories and then just the basic part of checkOrCleanArtifacts.
Please provide your feedback on this.
Query: If I use runShellCommand to remove folder '/sdcard/' what would happen?

[rpl]

Hi @Vishalghyv, here is another round of review comments (some of them are actually being pointed out in the previous review, but are not handled yet or not completely).

As a side note, I haven't looked into the tests yet because I'd like you to get to an implementation as simple and clean as possible first (and after that I'll review the code in the tests).

[rpl]

Thanks @Vishalghyv, this version looks already cleaner and better than the previous ones.
Follows another round of review comments for this new version.

[Vishalghyv]

Thanks @Vishalghyv, this version looks already cleaner and better than the previous ones.
Follows another round of review comments for this new version.

I mean first I thought my code was fine but following your suggestion was amazing. I couldn't work yesterday because of some errors in os. But will do today

[Vishalghyv]

Sir, Yes I tried to go through all four condition which might occur in this
if directories found or not and if clean is true or not -- Tried to optimize all four
And since dirs returned for readdir are in reversed order as in folder sdcard and artifacts are created at top thus reverse looping.
Please provide your feedback on it is helping a lot. Thanks by the way of such brief feedback

[Vishalghyv]

And since dirs returned for readdir are in reversed order as in folder sdcard and artifacts are created at top thus reverse looping.
This was required as in my case there were 30 folders in sdcard thus in some might have much more, Maybe this is not required?

[rpl]

@Vishalghyv it looks that we are getting there.
Follows another round of review comments.

[Vishalghyv]

Sir, I have tried to implement the requested changes.

And will change var directories to let directories in the next commit

[Vishalghyv]

Sorry, For so many commits and clean ups left
I tried to find a way to mock creation of dir during, so as to have some basic logic for testing,
This is the most useful approach I was able to use, by this approach almost all conditions for the function can be tested like with no directories or with remove condition false
But I am sure there must be a better way
And thanks for the great explanations!!

[rpl]

temp doesn't seem to be used anywhere else bug in the next line, doesn't seem necessary to give it a name.

[Vishalghyv]

Can't directly concatenate the strings thus this was required, and can't use concat function as it will add sdcard in end of artifactDir

[rpl]

not sure what you mean with tou can't use concat function? there is no real need to use different ways to concatenate strings (the String concat method and the + operator).

you can just compose all of them together using the template string syntax I mentioned in another review comment:

const part1 = "part1";
const part2 = "part2";
const part3 = "part3";
const aComposedString = `${part1}${part2}${part3}`;

[Vishalghyv]

Maybe four files are not necessary but this allowed to make sure that the test would break if the function did not worked properly especially when it's the case of deleting a folder from the users phone

[rpl]

I'm sorry @Vishalghyv took me longer than expected to get back to this and focus on it a bit to collect another round of review comments.

Follows some new review comments related to the changes on the implementation side and some more on the tests (as I also mentioned below I think that we also need some more tests in test.firefox-android.js to better cover this change).

We should also make sure to remove the changes to package-lock.json and package.json from this PR.

[rpl]

not sure what you mean with tou can't use concat function? there is no real need to use different ways to concatenate strings (the String concat method and the + operator).

you can just compose all of them together using the template string syntax I mentioned in another review comment:

const part1 = "part1";
const part2 = "part2";
const part3 = "part3";
const aComposedString = `${part1}${part2}${part3}`;

[Vishalghyv]

Should I use two tests for different params, two running instance or is there another way? (for checking the expected call params of checkOrCleanArtifacts function)
And except for checking expected params call and order of function call what else can I check

[Vishalghyv]

If the test might look like the current version, should this test be added to the one above it i.e in 'stops any running instances of the selected Firefox apk'

And should adbCleanArtifacts directly retrieved from params

[rpl]

@Vishalghyv this pull request still container the unrelated changes to package.json and package-lock.json, removing the file in a new commit isn't the right way to remove those changes.

I did cleaned up and rebased your branch locally and pushed here in my github fork here (in the branch named pr-1866-Vishalghyv-REBASE-DONE):

• rpl@38abec6

you can get the commit from there and overwrite this PR with it (it has to be in a branch with the same name of your and and then pushed into your github fork using --force to overwrite the branch, otherwise git will refuse to push the commit on the existing branch because it is not a new commit on top of the existing one)

If you are curious about how I achieved that, here is what I did locally:

• started an interactive git rebase of the entire branch on the parent commit of your first commit:
  git rebase -i 2f9456b^1
  where 2f9456b is the sha1 of your first commit, and ^1 means "the parent commit of the one specified on the left"

• git opens my editor with a list of the commit to rebase (the first column is the operation to run on the commit as part of the rebase, the second column is the sha1 of the commit), I marked the first commit as pick and all the others as fixup (to squash all the commits into the first one and discards their commit message):

pick 2f9456b fix: Cleanup old web-ext run artifacts dir when we are connecting to an android device
fixup <SHA1> next commit message
fixup <SHA1> other commit message
...
fixup <SHA1> last commit message

• at this point there is only a single commit with all the changes in the pull request, but the changes to package.json and package-lock.json are still there

• to remove those unrelated changes I asked git to restore the content of those file as "they where on the commit right above the current one" (because that is exactly your original branching point):

git checkout HEAD^1 -- package.json
git checkout HEAD^1 -- package-lock.json

• at this point there are changes to the two files (with their original content) pending to be committed, by amending the current commit we can get rid of the unrelated changes:

git commit --amend

• now the last step is to rebase the entire branch to the current master

git remote add upstream https://github.com/mozilla/web-ext
git fetch upstream
git rebase upstream/master

• at this point the branch should be right on top of the most recent commit in the master branch for this repository, which can also be verified by using git log --graph:

* commit 99797560a54e62e4166b0a83bf69501d971ee5d6 (HEAD -> Vishalghyv-new)
| Author: Vishhalghyv <vishal38785@gmail.com>
| Date:   Sat Mar 21 19:56:50 2020 +0530
| 
|     fix: Cleanup old web-ext run artifacts dir when we are connecting to an android device
| 
|  src/cmd/run.js                                            |  3 +++
|  src/extension-runners/firefox-android.js                  | 28 ++++++++++++++++++++++++++++
|  src/program.js                                            |  5 +++++
|  src/util/adb.js                                           | 42 +++++++++++++++++++++++++++++++++++++++++-
|  tests/unit/test-extension-runners/test.firefox-android.js | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
|  tests/unit/test-util/test.adb.js                          | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
|  6 files changed, 223 insertions(+), 1 deletion(-)
| 
* commit 7d315770b7064eaa445e1e171cdcedf9e4656c67 (upstream/master, upstream/HEAD, master)
| Author: Luca Greco <lgreco@mozilla.com>
| Date:   Wed May 6 00:25:50 2020 +0200
| 
|     chore(audit): Temporarily add yargs-parser adv url to nsprc
| 
|  .nsprc | 2 ++
|  1 file changed, 2 insertions(+)
| 
...

Mission accomplished! yay \o/

Using the revision system can be tricky some times, but the more you practice it (and then learn about it as a side effect), the easier and faster it gets to work with it.

I hope that this will help you to get the cleanup on this branch done.

Let me know if you have still issues and you are unable to get this branch cleaned up as needed.

[coveralls]

Coverage decreased (-0.07%) to 99.932% when pulling 88f74ff on Vishalghyv:new into 7d31577 on mozilla:master.

[coveralls]

Coverage decreased (-0.07%) to 99.932% when pulling 88f74ff on Vishalghyv:new into 7d31577 on mozilla:master.

[Vishalghyv]

Mission accomplished! yay \o/

[Vishalghyv]

Thanks Sir for the help :)

[Vishalghyv]

Tried the original version by you, Comment was really good, I saved it as a code snippet for squashing commits and rebase.

[rpl]

Closing as superseded by #1965 (which included a rebased version of the changes in this PR, with the author metadata preserved, plus some small tweaks and cleanups as described in #1965)