Closed Bug 1065988 Opened 7 years ago Closed 7 years ago

[mozversion] If platform.ini for a local application is missing, an exception should be raised

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: whimboo, Assigned: parkouss, Mentored)

References

()

Details

(Whiteboard: [lang=py])

Attachments

(1 file, 3 obsolete files)

From bug 1061809 comment 29:

> > > +    def test_without_platform_file(self):
> > > +        """With a missing sources file no exception should be thrown"""
> > 
> > The comment mentions sources file, but this is about the platform ini file.
> > Should we not raise an exception if the platform.ini file is not present?
> 
> This is something we haven't done yet, but it's a good question. I think
> it's also mandatory beside the application.ini file. Would you mind when we
> do this change in a new bug, given that it is not really related to this
> one? I would like to not even pack more underlying behavior changes into
> this patch. It already scares me. :)

So the behavior should be similar to the existent application.ini case. That means we only need some additional checks.
I think this is related, and maybe fix this: Bug 1068956.
This is about raising a LocalAppNotFound if application.ini is present but platform.ini is missing. Here's the relevant test that would need changing: http://hg.mozilla.org/mozilla-central/file/62f0b771583c/testing/mozbase/mozversion/tests/test_binary.py#l88
Hmm OK.

To be sure I fully understand, does that means that both application.ini and platform.ini are required ?
(In reply to Julien Pagès from comment #3)
> Hmm OK.
> 
> To be sure I fully understand, does that means that both application.ini and
> platform.ini are required ?

Yes, that's correct.
I ran the unit tests locally with success.
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Attachment #8507876 - Flags: review?(dave.hunt)
Comment on attachment 8507876 [details] [diff] [review]
If platform.ini for a local application is missing, an exception should be raised

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

There's an issue in the logic for OSX, otherwise just a couple of nits.

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +90,3 @@
>  
> +        if sys.platform == 'darwin':
> +            path = os.path.join(os.path.dirname(path), 'Resources')

This means that we'll always check in the Resources directory on OSX, whereas before we would first check in the directory above this first.

@@ +95,5 @@
> +        for name in ('application.ini', 'platform.ini'):
> +            filename = os.path.join(path, name)
> +            if not os.path.exists(filename):
> +                self._logger.warning('Unable to find %s' % filename)
> +                raise errors.LocalAppNotFoundError(path) 

Nit: Trailing whitespace.

::: testing/mozbase/mozversion/tests/test_sources.py
@@ +40,5 @@
>      def tearDown(self):
>          os.chdir(self.cwd)
>          mozfile.remove(self.tempdir)
>  
> +    def _write_conf_files(self, application=True, platform=True, sources=True):

I don't think we need application or platform to be optional.

@@ +71,5 @@
>                            self.binary, os.path.join(self.tempdir, 'invalid'))
>  
>      def test_without_sources_file(self):
>          """With a missing sources file no exception should be thrown"""
> +        self._write_conf_files(sources=None)

Nit: Can we make this sources=False as I think the intention is a little clearer.
Attachment #8507876 - Flags: review?(dave.hunt) → review-
Thanks Dave!

I added a test for osx Resources check path logic.
Attachment #8507876 - Attachment is obsolete: true
Attachment #8508127 - Flags: review?(dave.hunt)
Comment on attachment 8508127 [details] [diff] [review]
If platform.ini for a local application is missing, an exception should be raised

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

Looks good, please just address the couple of nits.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +90,5 @@
> +            # or if they are in the Resources dir
> +            resources_dir = os.path.join(self.tempdir, 'Resources')
> +            os.mkdir(resources_dir)
> +            for ini_file in ('application.ini', 'platform.ini'):
> +                shutil.move(os.path.join(self.tempdir, ini_file), resources_dir,)

Nit: Remove trailing comma

@@ +92,5 @@
> +            os.mkdir(resources_dir)
> +            for ini_file in ('application.ini', 'platform.ini'):
> +                shutil.move(os.path.join(self.tempdir, ini_file), resources_dir,)
> +
> +        

Nit: Remove whitespace.
Attachment #8508127 - Flags: review?(dave.hunt) → review+
Comment on attachment 8508127 [details] [diff] [review]
If platform.ini for a local application is missing, an exception should be raised

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +93,5 @@
> +                    and os.path.exists(os.path.join(path, 'platform.ini')))
> +
> +        if not check_location(path):
> +            if sys.platform == 'darwin':
> +                resources_path = os.path.join(path, 'Resources')

Hm, doesn't that construct a path like `Contents/MacOS/Resources`, while it should be `Contents/Resources`? Please test this with a v1 and v2 signed build of Firefox on OS X by passing in the binary (-b) to the test script.
Hm I think you're right. I don't know OSX, but looking at the previous code, I think it should be:

resources_path = os.path.join(os.path.dirname(path), 'Resources')

I missed the os.path.dirname part.

That means on OSX, the tree may look like this:

Contents/binary
Resources/platform.ini
Resources/application.ini

I assumed by mistake that it was like this:

Contents/binary
Contents/Resources/platform.ini
Contents/Resources/application.ini

I will fix it. Please tell me if there is an easy way to test as you recommended on a Linux laptop (I don't have any OSX).
(In reply to Julien Pagès from comment #10)
> That means on OSX, the tree may look like this:
> Contents/binary

I think you meant MacOS/binary here.
@Stephen
Yes thanks, I used Contents instead of MacOS in my previous comment.

@Henrik
Thanks for the link! It was what I thought, and current patch is broken. I'll rework on this.

Again, thank you. :)
I fixed the code, the broken test_with_ini_files_on_osx test, fixed the Nits and ran unit tests locally with success. Thanks Dave for the multiple reviews. :)
Attachment #8508127 - Attachment is obsolete: true
Attachment #8508787 - Flags: review?(dave.hunt)
Comment on attachment 8508787 [details] [diff] [review]
If platform.ini for a local application is missing, an exception should be raised

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

Other than the whitespace nit this looks good to me. Let's see if Henrik can give us some feedback too.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +94,5 @@
> +            os.mkdir(contents_dir)
> +            moved_binary = os.path.join(contents_dir,
> +                                        os.path.basename(self.binary))
> +            shutil.move(self.binary, moved_binary)
> +            

Nit: Please remove this extra whitespace.
Attachment #8508787 - Flags: review?(dave.hunt)
Attachment #8508787 - Flags: review+
Attachment #8508787 - Flags: feedback?(hskupin)
Comment on attachment 8508787 [details] [diff] [review]
If platform.ini for a local application is missing, an exception should be raised

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

::: testing/mozbase/mozversion/mozversion/mozversion.py
@@ +90,4 @@
>  
> +        def check_location(path):
> +            return (os.path.exists(os.path.join(path, 'application.ini'))
> +                    and os.path.exists(os.path.join(path, 'platform.ini')))

I would not put this method in the middle of the code, but define it at the top of this method before the first line of code starts. It's hard to find it that way.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +82,5 @@
> +        self._write_ini_files()
> +
> +        platform = sys.platform
> +        sys.platform = 'darwin'
> +        try:

IMHO we should not fake the binary on OS X but allow to run the tests with a real binary. Such a test is already present and gets only executed when you specify -b on the command line. Dave, what do you think?
Attachment #8508787 - Flags: feedback?(hskupin) → feedback+
(In reply to Henrik Skupin (:whimboo) from comment #16)
> IMHO we should not fake the binary on OS X but allow to run the tests with a
> real binary. Such a test is already present and gets only executed when you
> specify -b on the command line. Dave, what do you think?

I can see the advantage here as it tests the code path when running tests on another platform.
So let it be. Better to have too much coverage as too less.
Attachment #8510416 - Flags: review?(dave.hunt) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac33dde6856c

Thanks Julien!
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/ac33dde6856c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.