889471 - Buttons need customization tests

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P1)

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

Which kind of tests do you have in mind, Erik?
Buttons' test suite already has tests where the button is moved between navbar and panel for example – in order to test that the icon is properly set.

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #1)
> Which kind of tests do you have in mind, Erik?
> Buttons' test suite already has tests where the button is moved between
> navbar and panel for example – in order to test that the icon is properly
> set.

Where are these tests? perhaps they were added after I made this bug?

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)

> > Which kind of tests do you have in mind, Erik?
> > Buttons' test suite already has tests where the button is moved between
> > navbar and panel for example – in order to test that the icon is properly
> > set.
> 
> Where are these tests? 

https://github.com/mozilla/addon-sdk/blob/master/test/test-ui-action-button.js#L642

> perhaps they were added after I made this bug?

I don't think so, but as I said the tests were more about move the button to check that the proper icon was set depends by the area, both in regular and retina display, so maybe you have in mind some other kind of tests related to customization that we can implement.

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #2)
> 
> > > Which kind of tests do you have in mind, Erik?
> > > Buttons' test suite already has tests where the button is moved between
> > > navbar and panel for example – in order to test that the icon is properly
> > > set.
> > 
> > Where are these tests? 
> 
> https://github.com/mozilla/addon-sdk/blob/master/test/test-ui-action-button.
> js#L642
> 
> > perhaps they were added after I made this bug?
> 
> I don't think so, but as I said the tests were more about move the button to
> check that the proper icon was set depends by the area, both in regular and
> retina display, so maybe you have in mind some other kind of tests related
> to customization that we can implement.

This seems good enough, I suppose I missed that.  I'd prefer to have an explicit test, but since it'd do the same think as the one linked to above I see no reason to keep this open.

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

Ah I remember the type of test that I was expecting now.

Customization tests should include all of this:

1. moving the button from one place to another
2. opening a new window and ensuring the move is reflected in the new window
3. restarting the browser and ensuring that the move is still present.

Comment 6

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)

> Customization tests should include all of this:
> 
> 1. moving the button from one place to another

I would say that this is covered by the test above.

> 2. opening a new window and ensuring the move is reflected in the new window

We have already similar tests, but I'm not sure if we have assertion for the position. I will check and in case implement them. 

> 3. restarting the browser and ensuring that the move is still present.

Not sure how to test that. Do we have other tests that restarts the browser? Have you done the same test for sidebar? (If a sidebar is shown and then the browser is closed, when it's restarted the sidebar should be displayed again) If yes, can you point me to it, so I can use the same approach? If not, we should add that test too.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)

> Not sure how to test that. Do we have other tests that restarts the browser?
> Have you done the same test for sidebar? (If a sidebar is shown and then the
> browser is closed, when it's restarted the sidebar should be displayed
> again) If yes, can you point me to it, so I can use the same approach? If
> not, we should add that test too.

And we probably need a similar test for toolbar too, if it's not there.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #5)
> 
> > Customization tests should include all of this:
> > 
> > 1. moving the button from one place to another
> 
> I would say that this is covered by the test above.

Yeah I agree.

> > 2. opening a new window and ensuring the move is reflected in the new window
> 
> We have already similar tests, but I'm not sure if we have assertion for the
> position. I will check and in case implement them. 

Cool, I if it is missing then we can get an explicit test in the suite for this.

> > 3. restarting the browser and ensuring that the move is still present.
> 
> Not sure how to test that. Do we have other tests that restarts the browser?
> Have you done the same test for sidebar? (If a sidebar is shown and then the
> browser is closed, when it's restarted the sidebar should be displayed
> again) If yes, can you point me to it, so I can use the same approach? If
> not, we should add that test too.

This kind of test is probably missing for sidebars too, we should file a bug for it.  I think we can write a good enough test by creating a custom loader, using that to create a button, customize it, then unload the loader with a 'shutdown' reason, and create a new loader with a `loadReason == startup`

We should create a way to run these tests with actual restarts but there is no way to do that with our test framework atm.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #6)
> 
> > Not sure how to test that. Do we have other tests that restarts the browser?
> > Have you done the same test for sidebar? (If a sidebar is shown and then the
> > browser is closed, when it's restarted the sidebar should be displayed
> > again) If yes, can you point me to it, so I can use the same approach? If
> > not, we should add that test too.
> 
> And we probably need a similar test for toolbar too, if it's not there.

Can you make a bug for this, I'm not really sure what the expected result would be for toolbars.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

I made bug 991337 for the sidebar issue that you mentioned Matteo.

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)

> > > 3. restarting the browser and ensuring that the move is still present.

> This kind of test is probably missing for sidebars too, we should file a bug
> for it.  I think we can write a good enough test by creating a custom
> loader, using that to create a button, customize it, then unload the loader
> with a 'shutdown' reason, and create a new loader with a `loadReason ==
> startup`

Yes, it's not exactly restart the browser, but I can't image how do that in our current framework.

> We should create a way to run these tests with actual restarts but there is
> no way to do that with our test framework atm.

Yeah, that's why I said that I wasn't sure how to test that.

> Can you make a bug for this, I'm not really sure what the expected result
> would be for toolbars.

It should be exactly the same of sidebar, but I will double check and take care to file the bug for the toolbar. Thanks to opened the one for the sidebar!

Comment 12

[Tomislav Jovanovic :zombie]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> We should create a way to run these tests with actual restarts but there is
> no way to do that with our test framework atm.

doesn't our framework already run a fresh firefox for every test addon? we could document/make a hard and explicit guarantee on ordering (alpha sorted by name), set up the test conditions in TestAddon1, and test them in TestAddon2 (which will always run right after it).

it's quick and somewhat dirty, but not even close to biggest hack already present in the test framework..

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tomislav Jovanovic [:zombie] from comment #12)

> doesn't our framework already run a fresh firefox for every test addon?

We should create two test addons (so, to be clear, under `tests/addons`) for one check; ensure that they're run with the same profile and not a new one every time; and have an empiric solution to ensure the ordering.
I'm definitely -1 to that.

I would say that the best approach so far is using two loader in the same tests, until we have get rid of cfx: with the native jetpack, we could easier improve such aspect of our framework I believe, with less hack and empiric solution that the one here proposed.

> it's quick and somewhat dirty, but not even close to biggest hack already
> present in the test framework..

Yeah, it's a bit too dirty for me, and empiric. Personally I would prefer to avoid to introduce such thing. Unless we improve already our unit test framework to handle such case; but with the native jetpack on the way I'm not sure if it's worthy now.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Tomislav Jovanovic [:zombie] from comment #12)
> (In reply to Erik Vold [:erikvold] [:ztatic] from comment #8)
> > We should create a way to run these tests with actual restarts but there is
> > no way to do that with our test framework atm.
> 
> doesn't our framework already run a fresh firefox for every test addon? we
> could document/make a hard and explicit guarantee on ordering (alpha sorted
> by name), set up the test conditions in TestAddon1, and test them in
> TestAddon2 (which will always run right after it).
> 
> it's quick and somewhat dirty, but not even close to biggest hack already
> present in the test framework..

I think we should just wait for the native jetpack test suite and plan a real solution there.

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

Remove myself since I'm not working on SDK anymore, and Mozilla is going to focus on WebExt.

Comment 16

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562