896038 - mozinfo.find_and_update_from_json() ignores the dirs parameter if mozbuild is present

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Andrew Halberstadt [:ahal]]

See: https://github.com/mozilla/mozbase/blob/a659a23094695b8b5c93c0a540996b64a414c58a/mozinfo/mozinfo/mozinfo.py#L134

This causes test failures when running from an objdir:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25498269&tree=Try

We could just fix the tests, but I think silently ignoring the 'dirs' parameter is confusing. I propose that find_and_update_from_json() calls update on *all* mozinfo.json files it finds instead of returning after finding the first one. This way it would first populate mozinfo with the json found via mozbuild (if any) and then proceed to add any files it finds in the dirs parameter.

Does this make sense to everyone?

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: 0001-Bug-896038-Prevent-mozinfo-tests-from-using-build-ge.patch

Actually I'd really like to get feedback from ted on the above proposal, but unfortunately he is gone for awhile. I'll fix up the tests for now (because they are blocking bug 895940) and we can revisit whether comment 0 is worthwhile or not later.

Comment 2

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

Comment on attachment 778681 [details] [diff] [review]
0001-Bug-896038-Prevent-mozinfo-tests-from-using-build-ge.patch

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

this fix seems fine, ++ for a comment.

Comment 3

[Andrew Halberstadt [:ahal]]

https://github.com/mozilla/mozbase/commit/4777423874ce2dd91cbf09aba0caa942c7b6b6e1

Leaving open as per comment 1

Comment 4

[(not currently active) Ted Mielczarek]

This was intentional. It's supposed to use the in-tree configuration if it exists. I agree that it makes the API a little confusing.

I don't think that using multiple mozinfo.json files makes sense. I think your workaround in the tests is fine, since we just want to ensure that the API does what we want, and running from an objdir makes the "use the file from dirs" case tricky to test.

Comment 5

[Andrew Halberstadt [:ahal]]

Wfm.