1194996 - create a simple API to fetch build information for a build

Status: RESOLVED FIXED

(Testing :: mozregression, defect)

Description

[Julien Pagès (:parkouss)]

Right now, information related to a build (url, repo, changeset...) that we can find over internet is fetched inside build_data.py.

While this works well, this module is quite complicated and does two important things:

 - fetch build information for a build (given a fetch_config and a changeset/date)
 - manage the data range of a bisection (decide when to fetch info for a build for example, and what is the range)

So we should make this easier, by providing a simple API to only do the first point. I.E., refactor the code to be able to get build information for one build only - not using a range of builds.

This will also help if we want to fix bug 1194941.

Comment 1

[Julien Pagès (:parkouss)]

Attachment: fetch_build_info

So, asking Will for feedback, as it is quite a change, and may take time to review/test. Though, I would like to get your feedback on this, see if you like the overall changes. :)

Jonathan, asking you for a review. This is another quite big review, but I think less risky than the last one.

Well I tested with the basic use cases, it doe not seem to create regressions. If anybody want to test and give feedback, feel free to do so! That will help. :)

Comment 2

[jonathan.pigree]

Okay. As you said, this is a pretty big PR. I can't say, I validated everything, there is just too much code.
But I like the split you made into build_data. This class seems less scary now. The infos fetching part is encapsulated and it is easier to follow.

I tested it with a few cases (till the end of the bisections these times). I don't think this breaks something obvious.

But still I am uneasy now. Can we create an issue to add some non regression tests for mozregression? I mean no mock and unittests, we take mozregression and launch it with some entries and verify the results. With the --persists, I think it is atteignable now and not that long to launch. Actually, we can implement a quick non reg and a lengthy one. And store the golden files on a separate server. Perhaps, it is more complicated that I think (handling firefox shutdown and writing of verdict, etc...).

It will be complementary with unittests I think and I will be less scared when we merge big features like this one. If it is okay with you, I will create the bug.

One last thing, in build_data.py, bisector.py and now fetch_build_infos.py, I keep seeing access to dicts with hard coded keys. Can't we have some better defined data structures (like classes) instead? Or at least factorize those hard coded keys in constant (I think it is also a new bug to create).

Comment 3

[Julien Pagès (:parkouss)]

Yay, thanks a LOT Jonathan for looking at this. :)

(In reply to jonathan.pigree from comment #2)
> But still I am uneasy now. Can we create an issue to add some non regression
> tests for mozregression? I mean no mock and unittests, we take mozregression
> and launch it with some entries and verify the results. With the --persists,
> I think it is atteignable now and not that long to launch. Actually, we can
> implement a quick non reg and a lengthy one. And store the golden files on a
> separate server. Perhaps, it is more complicated that I think (handling
> firefox shutdown and writing of verdict, etc...).
> 
> It will be complementary with unittests I think and I will be less scared
> when we merge big features like this one. If it is okay with you, I will
> create the bug.

Well yes, this is a good idea. Maybe the --command argument can help to automatize
a regression this way. Feel free to open a bug and think about it, I think it is a
great idea. This will take some time, and this is not a priority for me, but it
would be a great tool to have.

> One last thing, in build_data.py, bisector.py and now fetch_build_infos.py,
> I keep seeing access to dicts with hard coded keys. Can't we have some
> better defined data structures (like classes) instead? Or at least factorize
> those hard coded keys in constant (I think it is also a new bug to create).

Yep, this is what is called everywhere "build_infos". This is a dict, that hold
all the information required for trying a build from downloading to testing.
It is created with the info fetching (the patch here) and will be used to download,
install, try the build.

I fully agree that a dict is not anymore a good solution. We should write a real class
for that, with documentation about the live cycle and so on. Feel free to open a bug
if you want to (else I will, one day or the other - probably soon!). This is something
important, and I think this requires to be addressed sooner than later.

Comment 4

[jonathan.pigree]

Okay. I created those bugs.

Yes, it will take time to create non regression tests. I am totally aware of that. But it will be an asset in the end and we do not have to rush it.

I also agree with how we should change the build_infos into a class.

Comment 5

[Julien Pagès (:parkouss)]

Okay, I merged this with https://github.com/mozilla/mozregression/commit/2e6f63ac7f7adddf0586489f0ddbbb68add704f9 since I really feel that it is a good thing to have. And we will be able to move on with some build info simplification now.

Will, feel free to look at the patch still! and if anything seems not good to you please say it so we can address it.

Comment 6

[William Lachance (:wlach)]

Comment on attachment 8648389 [details] [review]
fetch_build_info

This all makes sense to me. Some minor feedback on the comments in PR.