769082 - browserInfo should not use the magical string "NULL"

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

http://hg.mozilla.org/build/talos/file/0c2521dde365/talos/run_tests.py#l80

For 'repository' and 'sourcestamp' we use a special string "NULL":

http://hg.mozilla.org/build/talos/file/0c2521dde365/talos/run_tests.py#l203

Python has a singleton for this: None . browser_config should have
None as the default, then the logic can look as follows:

info = {'buildid': ('App', 'BuildID'),
        'repository': ('App', 'SourceRepository')
        'sourcestamp': ...,
        ...}

for key, value in info.items():
    if not browser_config.get(key):
       browser_config[key] = config.get(*value)

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

I thought we needed NULL for a reason, maybe to insert into the database?

Comment 2

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #1)
> I thought we needed NULL for a reason, maybe to insert into the database?

I haven't done a complete audit on what all would have to change.  It is likely that 'NULL' should be used in the graphserver report, but IMHO that is an issue of serialization and not a string we should store and check against internally

Comment 3

[William Lachance (:wlach)]

I ran into this when working on bug 1068989. I believe we can just take this out, all valid builds (even user builds) should have these variables defined. Any objections if I file this as INVALID and remove the code in question in bug 1068989?

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

good idea.  As long as we guarantee we have a value.

Comment 5

[William Lachance (:wlach)]

So this bug just resulted in a bunch of invalid information being submitted to Datazilla, which caused a whole bunch of confusion. I would like to fix this via bug 1068989, but I'm not sure when I'm going to finish that. Let's do up a quick patch to address this problem.

Comment 6

[William Lachance (:wlach)]

Attachment: Don't ever allow NULL values for repository or sourcestamp

Try run here:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=60b0e501e2a1

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8505776 [details] [diff] [review]
Don't ever allow NULL values for repository or sourcestamp

Review of attachment 8505776 [details] [diff] [review]:
-----------------------------------------------------------------

please ensure you test this- especially on android.  I don't like the hardcoded value, I assume that was a development/debugging thing.

::: talos/results.py
@@ +538,4 @@
>      # make a browser_config dictionary with minimal information
>      browser_config = {'branch_name': '',
>                        'buildid': 'testbuildid',
> +                      'sourcestamp': '62f0b771583c',

why the hardcoded value?

Comment 8

[William Lachance (:wlach)]

Comment on attachment 8505776 [details] [diff] [review]
Don't ever allow NULL values for repository or sourcestamp

Review of attachment 8505776 [details] [diff] [review]:
-----------------------------------------------------------------

::: talos/results.py
@@ +538,4 @@
>      # make a browser_config dictionary with minimal information
>      browser_config = {'branch_name': '',
>                        'buildid': 'testbuildid',
> +                      'sourcestamp': '62f0b771583c',

I agree that hardcoded values are normally bad, but this is just a test strawman. I don't think we ever shipped Firefox 3.14 :) I only removed NULL because I didn't want to give people the impression that it was acceptable value. :)

Comment 9

[William Lachance (:wlach)]

Comment on attachment 8505776 [details] [diff] [review]
Don't ever allow NULL values for repository or sourcestamp

Review of attachment 8505776 [details] [diff] [review]:
-----------------------------------------------------------------

Resetting r?, as I think the r- was due to a misunderstanding

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

my concern is that we might be uploading 62f0b771583c as the sourcestamp instead of NULL; that will cause other problems in and of itself.  Am I reading this wrong?

Comment 11

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #10)
> my concern is that we might be uploading 62f0b771583c as the sourcestamp
> instead of NULL; that will cause other problems in and of itself.  Am I
> reading this wrong?

Yes, you are looking at test code which is not used in production.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8505776 [details] [diff] [review]
Don't ever allow NULL values for repository or sourcestamp

Review of attachment 8505776 [details] [diff] [review]:
-----------------------------------------------------------------

/me not dense anymore

Comment 13

[William Lachance (:wlach)]

Pushed: https://hg.mozilla.org/build/talos/rev/aee1aa1e3f02