1195390 - mozregression should create the persist folder if it does not exists

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

So everything is in the title. If a persist folder is given and that it does not exists, we should create it automatically.

Comment 1

[jonathan.pigree]

As discussed. I take this bug.

Comment 2

[Julien Pagès (:parkouss)]

So, Jonathan is busy with other things. Let's see if someone else is interested to look into this!

Comment 3

[Sumeet (:gopianis)]

Hi there, I'm a Coventry University student looking to do this for my first bug. Is that ok?

Comment 4

[Julien Pagès (:parkouss)]

Hey GopianiS, sure!

So basically you will need to first create a fork of the project on github, https://github.com/mozilla/mozregression.

You can get some 'getting started' information of how to contribute from there:
http://mozilla.github.io/mozregression/contribute.html

For the purpose of this bug, the persist folder is what we pass from the command line using the "--persist" flag. We also use it in the GUI (https://github.com/mozilla/mozregression/tree/master/gui) but we can look at that in a second step. I let you start with this information, please ask here if you have any question.

We can also chat on irc, https://wiki.mozilla.org/IRC - my nickname is parkouss.

Comment 5

[Sumeet (:gopianis)]

Okay, I've made a fork https://github.com/GopianiS/mozregression

I've already gone through the setup, Will I need to specify good and bad params in my test or will I be able to execute "mozregression --persist"?

Comment 6

[MikeLing]

(In reply to GopianiS from comment #5)
Hi GopianiS, it's always nice to hear from newcomer. :)

> I've already gone through the setup, Will I need to specify good and bad
> params in my test or will I be able to execute "mozregression --persist"?

actually, I think the --persist is to set the path of persist folder, and the main idea of this issue is when you launch mozregression, it will check if the folder exist and it will create one if the answer is no.  
So, the easiest way to test is you could launch mozregression to ensure your have a workable path for persist. Then you need to delete the folder and run it again to see if the folder will been created exactly in the same path. :)

after all, I'm totally defer to Julien's opinion here, he is know mozregression much better than I did.

Comment 7

[Julien Pagès (:parkouss)]

(In reply to GopianiS from comment #5)
> Okay, I've made a fork https://github.com/GopianiS/mozregression
> 
> I've already gone through the setup, Will I need to specify good and bad
> params in my test or will I be able to execute "mozregression --persist"?

If you run "mozregression --persist /path/to/folder", there will be some defaults for the good and bad dates, so you can omit them in the command line.

For example you can just run:

> mozregression --persist /path/to/invalid/dir

and mozregression will fail - this is what we should fix. Oh, BTW, do not pay attention to the warning you will see ("You are using mozregression version 1.0.1") if you are on master, you use the up to date mozregression.

Comment 8

[Julien Pagès (:parkouss)]

GopianiS , I think that handling the persist dir creation in the DownloadManager initialization [1] should be good. And it should automatically propagate to the GUI, so I believe it is our best option.

[1] https://github.com/mozilla/mozregression/blob/master/mozregression/download_manager.py#L187

Comment 9

[Sumeet (:gopianis)]

Hi Julien, 
Apologies for taking so long getting this bug fixed, I've had other assignments to complete.

I have hooked into an if statement in the download function https://github.com/GopianiS/mozregression/blob/master/mozregression/download_manager.py#L266 and am creating the folder before the download to the destination directory.

Hows this?

Comment 10

[Julien Pagès (:parkouss)]

(In reply to GopianiS from comment #9)
> Hi Julien, 
> Apologies for taking so long getting this bug fixed, I've had other
> assignments to complete.

No problem! Thanks for sticking with that bug.

> I have hooked into an if statement in the download function
> https://github.com/GopianiS/mozregression/blob/master/mozregression/
> download_manager.py#L266 and am creating the folder before the download to
> the destination directory.
> 
> Hows this?

Well, that is not good because the dest dir you are creating uses the full file path, not the directory path (from https://github.com/GopianiS/mozregression/blob/master/mozregression/download_manager.py#L232).

So basically you would create a directory every time we want to download a file.

Instead, the constructor knows about the directory path. So you could definitely put your if in there! Plus it would be more appropriate since we would check only when the instance is created, and not every time a file is downloaded.

Comment 11

[Sumeet (:gopianis)]

That makes sense, putting it in the constructor is a better place.
https://github.com/GopianiS/mozregression/blob/master/mozregression/download_manager.py#L232

Comment 12

[Julien Pagès (:parkouss)]

Looks good!
Can you create a github Pull request with your commit for that, and then create an attachment in this bug pointing at the pull request please ?

Please read the following:

http://ateam-bootcamp.readthedocs.org/en/latest/reference/git_github.html
http://ateam-bootcamp.readthedocs.org/en/latest/guide/development_process.html

Comment 13

[Sumeet (:gopianis)]

Attachment: Pull request for persist bug

Comment 14

[Julien Pagès (:parkouss)]

GopianiS, almost there!

You missed to ask for a review - you can do that now:

 - click on [details] for your attachment
 - select "?" in the review flag
 - hit my email address in the field, or hit ":parkouss" then choose the completion choice
 - click submit

Also, if you look at https://github.com/mozilla/mozregression/pull/350, you can see that the checks have failed.
You can click on details for the travis build for example, you would see some minor style issues you need to fix:
https://travis-ci.org/mozilla/mozregression/builds/91565493#L281

And I added some comments on github.

Please ask for a review once you fixed the nits. :)

Last thing, if you know how to squash commits with git, please do. Only one commit would be nice here. If you don't, no problem I can do that for you before merging - and we can discuss later on irc if you want me to explain how you can do that for next time.

Looking forward for the update!

Comment 15

[Julien Pagès (:parkouss)]

Comment on attachment 8688344 [details] [review]
Pull request for persist bug

Thannks, everything looks good now, and it works well!

Comment 16

[Julien Pagès (:parkouss)]

Sorry for the delay - I was waiting for the r? flag.

Anyway, this is all good now - I squashed your commits and landed the patch in https://github.com/mozilla/mozregression/commit/34825b79ebe546eb282e92b22af3177e89dde695.

Thanks for your time and the fix!

If you're interested for some other bugs in mozregression, tell me, there is some work to do on the GUI side (python wit PyQt4, this is a desktop application).