Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10237009:
Fix the UI label for verifying flash plugin status. (Closed)
|
Created:
8 years, 8 months ago by vivianz Modified:
8 years, 7 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionFix the UI label for verifying flash plugin status and re-enable this test.
BUG=125062
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134412
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 25 (0 generated)
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), are there many of these 'plugin-enabled' elements? If so, how exactly are you verifying that a new one got created?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 18:45:05, Nirnimesh wrote: > are there many of these 'plugin-enabled' elements? If so, how exactly are you > verifying that a new one got created? there used to be 2 set plugin-enabled and plugin-file-enabled, when you enable flash plugin both will be enabled, the dev just did a combine of the 2 since it does not make sense to have two sets, it's redundant. for my test I just need to verify the class label flipped to plugin-enabled. used to have 1 called plugin-file-enabled , now we have 2 both plugin-enabled. but they don't affect this test.
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 18:50:58, vivianz wrote: > On 2012/04/26 18:45:05, Nirnimesh wrote: > > are there many of these 'plugin-enabled' elements? If so, how exactly are you > > verifying that a new one got created? > there used to be 2 set plugin-enabled and plugin-file-enabled, when you enable > flash plugin both will be enabled, the dev just did a combine of the 2 since it > does not make sense to have two sets, it's redundant. for my test I just need to > verify the class label flipped to plugin-enabled. used to have 1 called > plugin-file-enabled , now we have 2 both plugin-enabled. but they don't affect > this test. I understand what you're trying to do, but I don't see from the test how you're verifying that the flip happened? Is the count 0 before the click on line 140? Why did you change line 143 to >=1?
+dyu
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), Do they both need to be enabled? Are the plugins exclusive or inclusive of each other? Trying to figure out why you changed from '==' to '=>'. Does it mean that at any given time one plugin can be enabled while the other is disabled or both can be enabled? On 2012/04/26 18:50:58, vivianz wrote: > On 2012/04/26 18:45:05, Nirnimesh wrote: > > are there many of these 'plugin-enabled' elements? If so, how exactly are you > > verifying that a new one got created? > there used to be 2 set plugin-enabled and plugin-file-enabled, when you enable > flash plugin both will be enabled, the dev just did a combine of the 2 since it > does not make sense to have two sets, it's redundant. for my test I just need to > verify the class label flipped to plugin-enabled. used to have 1 called > plugin-file-enabled , now we have 2 both plugin-enabled. but they don't affect > this test.
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), yes, before the click on line 140, the class name is plugin-disabled On 2012/04/26 19:06:14, Nirnimesh wrote: > On 2012/04/26 18:50:58, vivianz wrote: > > On 2012/04/26 18:45:05, Nirnimesh wrote: > > > are there many of these 'plugin-enabled' elements? If so, how exactly are > you > > > verifying that a new one got created? > > there used to be 2 set plugin-enabled and plugin-file-enabled, when you > enable > > flash plugin both will be enabled, the dev just did a combine of the 2 since > it > > does not make sense to have two sets, it's redundant. for my test I just need > to > > verify the class label flipped to plugin-enabled. used to have 1 called > > plugin-file-enabled , now we have 2 both plugin-enabled. but they don't affect > > this test. > > I understand what you're trying to do, but I don't see from the test how you're > verifying that the flip happened? > > Is the count 0 before the click on line 140? > Why did you change line 143 to >=1? http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), because now after developer make plugin-file-[something] folder deprecated. now both named plugin-[something], so i have 2 now, that is the reason i change it to >=. On 2012/04/26 19:14:52, dyu1 wrote: > Do they both need to be enabled? Are the plugins exclusive or inclusive of each > other? Trying to figure out why you changed from '==' to '=>'. Does it mean that > at any given time one plugin can be enabled while the other is disabled or both > can be enabled? > > On 2012/04/26 18:50:58, vivianz wrote: > > On 2012/04/26 18:45:05, Nirnimesh wrote: > > > are there many of these 'plugin-enabled' elements? If so, how exactly are > you > > > verifying that a new one got created? > > there used to be 2 set plugin-enabled and plugin-file-enabled, when you > enable > > flash plugin both will be enabled, the dev just did a combine of the 2 since > it > > does not make sense to have two sets, it's redundant. for my test I just need > to > > verify the class label flipped to plugin-enabled. used to have 1 called > > plugin-file-enabled , now we have 2 both plugin-enabled. but they don't affect > > this test. >
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 22:08:58, vivianz wrote: > because now after developer make plugin-file-[something] folder deprecated. now > both named plugin-[something], so i have 2 now, that is the reason i change it > to >=. then why not match to 2? Why >= 1?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), I could, I make it >=1 in case dev make another one or get rid of one of them in the future, the test will be more robust this way, since it only need to make sure one class label is flipped, how many of them them exist is irrelevant here. On 2012/04/26 22:19:58, Nirnimesh wrote: > On 2012/04/26 22:08:58, vivianz wrote: > > because now after developer make plugin-file-[something] folder deprecated. > now > > both named plugin-[something], so i have 2 now, that is the reason i change it > > to >=. > > then why not match to 2? Why >= 1?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 22:30:51, vivianz wrote: > I could, I make it >=1 in case dev make another one or get rid of one of them in > the future, the test will be more robust this way, since it only need to make > sure > one class label is flipped, how many of them them exist is irrelevant here. Before the click happens, what's the count? 1? If so, you should match for 2 here, or else this check is a no-op. Or did I totally misinterpret what's going on here?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), before the click , the class name is plugin-disabled, after the click the class name is plugin-enabled . so if click the enable button and nothing happen ( bug scenario) , the class name will not be flipped , so checking here will be count 0, therefore fail the test. so the correct way is before click, this one count 0, after click this one count >=0. On 2012/04/26 22:33:39, Nirnimesh wrote: > On 2012/04/26 22:30:51, vivianz wrote: > > I could, I make it >=1 in case dev make another one or get rid of one of them > in > > the future, the test will be more robust this way, since it only need to make > > sure > > one class label is flipped, how many of them them exist is irrelevant here. > > Before the click happens, what's the count? 1? > If so, you should match for 2 here, or else this check is a no-op. > > Or did I totally misinterpret what's going on here?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 22:42:56, vivianz wrote: > before the click , the class name is plugin-disabled, after the click the class > name is plugin-enabled . so if click the enable button and nothing happen ( bug > scenario) , the class name will not be flipped , so checking here will be count > 0, therefore fail the test. so the correct way is before click, this one count > 0, after click this one count >=0. I think you should do a strict match. ie == 1 here.
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), I can make it ==2, but I just don't see the point of doing so, it only make the test more flaky. can you tell me what is the benefit to doing so? I don't want to have to come back fix it again if few days later dev decide the other folder is redundant after all and removed it. On 2012/04/26 23:00:07, Nirnimesh wrote: > On 2012/04/26 22:42:56, vivianz wrote: > > before the click , the class name is plugin-disabled, after the click the > class > > name is plugin-enabled . so if click the enable button and nothing happen ( > bug > > scenario) , the class name will not be flipped , so checking here will be > count > > 0, therefore fail the test. so the correct way is before click, this one count > > 0, after click this one count >=0. > > I think you should do a strict match. ie == 1 here.
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 23:15:04, vivianz wrote: > I can make it ==2, but I just don't see the point of doing so, it only make the > test more flaky. can you tell me what is the benefit to doing so? I don't want > to have to come back fix it again if few days later dev decide the other folder > is redundant after all and removed it. I want to ensure that reality is coded in here. Options: if before click, count of plugin-enabled is 0, check for >0 after click I don't understand where ==2 comes from, maybe that's leading to the confusion. Also, line 142 is looking for elements only in the flash dom element. why would you expect there to be more than 1 plugin-enabled items?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), got it, I will change it to >0 tomorrow, that make sense, thanks for the point! On 2012/04/26 23:21:33, Nirnimesh wrote: > On 2012/04/26 23:15:04, vivianz wrote: > > I can make it ==2, but I just don't see the point of doing so, it only make > the > > test more flaky. can you tell me what is the benefit to doing so? I don't want > > to have to come back fix it again if few days later dev decide the other > folder > > is redundant after all and removed it. > > I want to ensure that reality is coded in here. > Options: > if before click, count of plugin-enabled is 0, check for >0 after click > > I don't understand where ==2 comes from, maybe that's leading to the confusion. > > Also, line 142 is looking for elements only in the flash dom element. why would > you expect there to be more than 1 plugin-enabled items?
http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py File functional/about_plugins_ui.py (right): http://codereview.chromium.org/10237009/diff/1/functional/about_plugins_ui.py... functional/about_plugins_ui.py:143: './/ancestor::*[@class="plugin-enabled"]')) >= 1), On 2012/04/26 23:21:33, Nirnimesh wrote: > On 2012/04/26 23:15:04, vivianz wrote: > > I can make it ==2, but I just don't see the point of doing so, it only make > the > > test more flaky. can you tell me what is the benefit to doing so? I don't want > > to have to come back fix it again if few days later dev decide the other > folder > > is redundant after all and removed it. > > I want to ensure that reality is coded in here. > Options: > if before click, count of plugin-enabled is 0, check for >0 after click > > I don't understand where ==2 comes from, maybe that's leading to the confusion. > > Also, line 142 is looking for elements only in the flash dom element. why would > you expect there to be more than 1 plugin-enabled items? Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/10237009/13001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/10237009/13001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/10237009/21004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/10237009/21004
Try job failure for 10237009-21004 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vivianz@chromium.org/10237009/21004
GOing to commit by hand on your behalf.
On 2012/04/27 23:27:48, Nirnimesh wrote: > GOing to commit by hand on your behalf. thanks a lot! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
