974694 - Need tool to interpret omni_analyzer results

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

We have an initial version of the fxos-certsuite; one of the outputs of this is a list of files that have changed from a reference version.

We need a corresponding tool that engineers can run on this output that will generate diffs against the reference version for easy comparison.

Comment 1

[Andreas Tolfsen ❲:ato❳]

If the JSON output is indented (which it is) can't we use the diff program for this job?  The expect results are found under certsuite/expected_omni_results.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

the json output just lists the filename and its base64encoded contents, so we can't it to give us a line-by-line diff of the files. We'd have to decode it and then run the diff

Comment 3

[cmtalbert]

Yes, exactly what mdas said. I did verify that base64 encoding preserved the newlines so when you decode the file and diff it with a canonical version, you should get a nice diff.

Let me know if you have any questions

Comment 4

[Jonathan Griffin (:jgriffin)]

Our Sprint 1 (March 17th) deliverable here is:

* A standalone tool in the https://etherpad.mozilla.org/fxos-cert-suite repo which can be run against the test output file to produce a diff of files from omni.jar that have changed.

Comment 5

[Dan Minor [:dminor]]

Attachment: Add omni_diff.py tool

I changed the expected results to use relative paths instead of just filenames. I updated the 1.3 expected results based on this, but didn't have a phone handy to do the 1.4 results.

I tested this using a local build of omni.ja for v1.3 which had a few differences from the phone version.

I wasn't sure if we wanted to add versions of omni.ja to the test suite or have people rely upon a local copy.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Dan Minor [:dminor] from comment #5)
> Created attachment 8386939 [details] [diff] [review]
> Add omni_diff.py tool
> 
> I changed the expected results to use relative paths instead of just
> filenames. I updated the 1.3 expected results based on this, but didn't have
> a phone handy to do the 1.4 results.
> 
> I tested this using a local build of omni.ja for v1.3 which had a few
> differences from the phone version.
> 
> I wasn't sure if we wanted to add versions of omni.ja to the test suite or
> have people rely upon a local copy.

fwiw, I think we should include the omni.ja in the certsuite/expected_omni_results/ directory, since the json file there is already going to be derived from that particular omni.ja. It'll help run our diffs locally without having to find the correct omni.ja from some other resource

Comment 7

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8386939 [details] [diff] [review]
Add omni_diff.py tool

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

Will r+ once we have confirmation on behaviour regarding new files and args are sorted out. Looks good and works well otherwise!

::: certsuite/omni_diff.py
@@ +43,5 @@
> +                for path in pathes:
> +                    reason = pathes[path]['reason']
> +                    if reason == 'PARTNER_FILE':
> +                        # Partner added this file, so no diff
> +                        continue

I thought we had to include new files in the diff as well? Seems reasonable since they can add components or other files to override behaviour in unchanged files.

@@ +75,5 @@
> +
> +def main(argv):
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument("resultspath", help="Path to file containing results of the omni analyzer",
> +        default=os.path.join(os.getcwd(), "results.json"))

it should be "--resultspath" if you want to let this be an optional argument. Otherwise, it will demand I pass it in. The same issue goes for "omnipath" and "outputpath"

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

I asked jgriffin in IRC and he said that the diff should include new files.

Comment 9

[Dan Minor [:dminor]]

Attachment: Add omni diff.

Thanks for the review, this removes the defaults for the command line (I don't want the parameters to be optional) and adds diffs for new files.

I agree about adding omni.ja to the expected results dir, I'll do that as part of landing this.

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8388548 [details] [diff] [review]
Add omni diff.

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

A few problems:

1) I applied this patch, added 2 new (empty) files to chrome/ and components/, modified a file and removed 2 files. When I ran with 'source run.sh', the results.json file only contained 3 changes, the 2 new files and my modification to a file, but did not include the files I removed. This should be produced so we can see missing files.

2) I wanted to see if I could get the new files in the outputted diff, but when I ran omni_diff.py on the results, I got:

  inflating: /Users/mdas/Code/fxos-certsuite/certsuite/omnidir/chrome/pt-BR/locale/pt-BR/b2g-l10n/overrides/search/search.properties
  inflating: /Users/mdas/Code/fxos-certsuite/certsuite/omnidir/chrome/pt-BR/locale/pt-BR/b2g-l10n/overrides/update/updates.properties
warning: identical diff for file: chrome/newfile.js
warning: identical diff for file: components/NEW.js
There were problems running diff. Are you certain you specified the correct omni.ja?

As a result, the diff didn't get produced. FYI, I ran omni_diff.py like : `python omni_diff.py /Users/mdas/Code/fxos-certsuite/results.json /Users/mdas/Code/fxos-certsuite/omni.ja.1.3 out.diff` which I think is what I needed to do.

::: certsuite/omni_diff.py
@@ +31,5 @@
> +        unzip_omnifile(self.omnipath, self.workdir)
> +
> +        # read results json
> +        with open(self.resultspath, 'r') as f:
> +            json_results = json.loads(f.read())

the results.json file that's output by the full certification suite will store the json you want to consume in a giant json blob, with the key "omni_result". You should check if "omni_result" is a key in json_results, and use its value as json_results if it is present.

Comment 11

[Dan Minor [:dminor]]

(In reply to Malini Das [:mdas] from comment #10)
> 2) I wanted to see if I could get the new files in the outputted diff, but
> when I ran omni_diff.py on the results, I got:
> 
>   inflating:
> /Users/mdas/Code/fxos-certsuite/certsuite/omnidir/chrome/pt-BR/locale/pt-BR/
> b2g-l10n/overrides/search/search.properties
>   inflating:
> /Users/mdas/Code/fxos-certsuite/certsuite/omnidir/chrome/pt-BR/locale/pt-BR/
> b2g-l10n/overrides/update/updates.properties
> warning: identical diff for file: chrome/newfile.js
> warning: identical diff for file: components/NEW.js
> There were problems running diff. Are you certain you specified the correct
> omni.ja?
> 
> As a result, the diff didn't get produced. FYI, I ran omni_diff.py like :
> `python omni_diff.py /Users/mdas/Code/fxos-certsuite/results.json
> /Users/mdas/Code/fxos-certsuite/omni.ja.1.3 out.diff` which I think is what
> I needed to do.
> 

Can you confirm that /Users/mdas/Code/fxos-certsuite/omni.ja.1.3 does not contain the two new files that you added?

Comment 12

[Dan Minor [:dminor]]

Attachment: Add omni_diff.py

I've added a check for removed files to omni_analyzer.py. I removed the check for empty diffs from omni_diff.py as this would cause empty files to be skipped. I added support for the overall results.json structure from run.sh.

I tested this by unzipping omni.ja, adding two new files, one with content, one empty, removing a file and modifying a file, rezipping and pushing to the device.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8390463 [details] [diff] [review]
Add omni_diff.py

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

Cool, almost there: In the output, the diff is reversed, meaning if I remove a file, the diff shows that the file was added instead. r+ when this is fixed. My results.json show that the file was removed, but I think that diff is just being called with the parameters in the wrong order.

Comment 14

[Dan Minor [:dminor]]

Attachment: Add omni_diff.py

Switched the order of arguments to diff.

Comment 15

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8390935 [details] [diff] [review]
Add omni_diff.py

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

great thanks! You may need to rebase to apply this patch to fxos-certsuite since logging has been added to the omni_analyzer

Comment 16

[Jonathan Griffin (:jgriffin)]

I unbitrotted and landed this:  https://github.com/mozilla-b2g/fxos-certsuite/commit/8549347ec0a52b67179ab62eb5c4d2e71e18c13a

Comment 17

[Jonathan Griffin (:jgriffin)]

We'll need to check in omni.ja too, we can handle that separately.