So as you probably know, some of our tests that pass in Firefox fail in Chrome.
One issue is that in a number of places the tests accept a string to look for in a URL that is defined as "Main Page". But in the browser, the URL ultimately will contain the string "Main_Page".
I committed a change such that the test would check that *at some point* the URL will contain the string "Main Page" as specified in the test. In hindsight, this was probably a bad idea, because the original tests just care that we land on the correct URL, not that we handle spaces correctly. https://gerrit.wikimedia.org/r/#/c/119878
Jon committed a change that invisibly munges the input string to replace the first (and only the first) space character with an underline character. This is probably also not a great idea, because it secretly changes test inputs without the user (or maintainer) knowing. https://gerrit.wikimedia.org/r/#/c/120564
After thinking it over, I think what we should do to make the tests perform the way the tests were intended to perform is to revert the two changes I mentioned above, and to simply change every occurrence of "Main Page" in the features to be "Main_Page". That will not only satisfy Firefox and Chrome, but it will also satisfy the intent of the tests themselves. I suspect that using "Main Page" was essentially a typo that propagated through the test suite over time.
-Chris
The danger with this is that "Main Page" and other forms will sneak in again. It is much more natural to write I am on the "San Francisco" page or I am on the "List of districts and neighborhoods of Los Angeles" page or I am on the "1+1" page
Whether this is a typo or not I'm not sure. There are various other replacements that occur to URLs other than ' ' => '_' - for example - 1+1 gets changed to "1%2B1" do we really propose that the writer of the test is aware of all of these? It seems like it goes against the idea of a feature being human readable and easy to write. This is why I proposed automatically substituting ' ' for '_' in my change.
Either way it sounds like both of these patches should be reverted as both seem to be sub-par. We should get consensus before fixing this.
On a side note: RIght now I'm much more interested in getting https://bugzilla.wikimedia.org/show_bug.cgi?id=62614 addressed first...
On Tue, Mar 25, 2014 at 12:15 PM, Chris McMahon cmcmahon@wikimedia.org wrote:
So as you probably know, some of our tests that pass in Firefox fail in Chrome.
One issue is that in a number of places the tests accept a string to look for in a URL that is defined as "Main Page". But in the browser, the URL ultimately will contain the string "Main_Page".
I committed a change such that the test would check that *at some point* the URL will contain the string "Main Page" as specified in the test. In hindsight, this was probably a bad idea, because the original tests just care that we land on the correct URL, not that we handle spaces correctly. https://gerrit.wikimedia.org/r/#/c/119878
Jon committed a change that invisibly munges the input string to replace the first (and only the first) space character with an underline character. This is probably also not a great idea, because it secretly changes test inputs without the user (or maintainer) knowing. https://gerrit.wikimedia.org/r/#/c/120564
After thinking it over, I think what we should do to make the tests perform the way the tests were intended to perform is to revert the two changes I mentioned above, and to simply change every occurrence of "Main Page" in the features to be "Main_Page". That will not only satisfy Firefox and Chrome, but it will also satisfy the intent of the tests themselves. I suspect that using "Main Page" was essentially a typo that propagated through the test suite over time.
-Chris
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
On Tue, Mar 25, 2014 at 2:04 PM, Jon Robson jrobson@wikimedia.org wrote:
The danger with this is that "Main Page" and other forms will sneak in again. It is much more natural to write I am on the "San Francisco" page
This is why I proposed automatically substituting ' ' for '_' in my change.
I haven't had this argument in a while, but I actually think checking for URL correctness is almost never a very good test. Do y'all think we could just flat remove some of these? (It is the one in commons_steps.rb that is causing the immediate problem, but some of those others aren't too smart either.)
$ grep -r "The URL of the page" * features/mainmenu.feature: Then The URL of the page should contain "Main_Page" features/mainmenu.feature: Then The URL of the page should contain "campaign=random" features/mainmenu.feature: Then The URL of the page should contain "Special:Nearby" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AWatchlist" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AUploads" features/mainmenu.feature: Then The URL of the page should contain "Special:MobileOptions" features/mainmenu.feature: Then The URL of the page should contain "Special:UserLogin" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/search.feature: And The URL of the page should contain "Main%20Page" features/search.feature: And The URL of the page should contain "Main%20Page" features/editor_ve.feature: And The URL of the page should contain "#/VisualEditor/1" features/step_definitions/common_steps.rb:Then(/^The URL of the page should contain "(.+)"$/) do |article| features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc" features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc"
I actually agree with you on this Chris. The URL correctness is not a good test. Looking at all of these I can see they don't really add anything to these tests and the mainmenu.feature tests could be written better. I'm preparing a patch as we speak to remove/rewrite these.
On Tue, Mar 25, 2014 at 3:37 PM, Chris McMahon cmcmahon@wikimedia.org wrote:
On Tue, Mar 25, 2014 at 2:04 PM, Jon Robson jrobson@wikimedia.org wrote:
The danger with this is that "Main Page" and other forms will sneak in again. It is much more natural to write I am on the "San Francisco" page
This is why
I proposed automatically substituting ' ' for '_' in my change.
I haven't had this argument in a while, but I actually think checking for URL correctness is almost never a very good test. Do y'all think we could just flat remove some of these? (It is the one in commons_steps.rb that is causing the immediate problem, but some of those others aren't too smart either.)
$ grep -r "The URL of the page" * features/mainmenu.feature: Then The URL of the page should contain "Main_Page" features/mainmenu.feature: Then The URL of the page should contain "campaign=random" features/mainmenu.feature: Then The URL of the page should contain "Special:Nearby" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AWatchlist" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AUploads" features/mainmenu.feature: Then The URL of the page should contain "Special:MobileOptions" features/mainmenu.feature: Then The URL of the page should contain "Special:UserLogin" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/search.feature: And The URL of the page should contain "Main%20Page" features/search.feature: And The URL of the page should contain "Main%20Page" features/editor_ve.feature: And The URL of the page should contain "#/VisualEditor/1" features/step_definitions/common_steps.rb:Then(/^The URL of the page should contain "(.+)"$/) do |article| features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc" features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc"
Done. https://gerrit.wikimedia.org/r/121102
On Wed, Mar 26, 2014 at 9:36 AM, Jon Robson jrobson@wikimedia.org wrote:
I actually agree with you on this Chris. The URL correctness is not a good test. Looking at all of these I can see they don't really add anything to these tests and the mainmenu.feature tests could be written better. I'm preparing a patch as we speak to remove/rewrite these.
On Tue, Mar 25, 2014 at 3:37 PM, Chris McMahon cmcmahon@wikimedia.org wrote:
On Tue, Mar 25, 2014 at 2:04 PM, Jon Robson jrobson@wikimedia.org wrote:
The danger with this is that "Main Page" and other forms will sneak in again. It is much more natural to write I am on the "San Francisco" page
This is why
I proposed automatically substituting ' ' for '_' in my change.
I haven't had this argument in a while, but I actually think checking for URL correctness is almost never a very good test. Do y'all think we could just flat remove some of these? (It is the one in commons_steps.rb that is causing the immediate problem, but some of those others aren't too smart either.)
$ grep -r "The URL of the page" * features/mainmenu.feature: Then The URL of the page should contain "Main_Page" features/mainmenu.feature: Then The URL of the page should contain "campaign=random" features/mainmenu.feature: Then The URL of the page should contain "Special:Nearby" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AWatchlist" features/mainmenu.feature: Then The URL of the page should contain "returnto=Special%3AUploads" features/mainmenu.feature: Then The URL of the page should contain "Special:MobileOptions" features/mainmenu.feature: Then The URL of the page should contain "Special:UserLogin" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/editor_wikitext_nosave.feature: And The URL of the page should contain "Nonexistent_page_ijewrcmhvg34773" features/search.feature: And The URL of the page should contain "Main%20Page" features/search.feature: And The URL of the page should contain "Main%20Page" features/editor_ve.feature: And The URL of the page should contain "#/VisualEditor/1" features/step_definitions/common_steps.rb:Then(/^The URL of the page should contain "(.+)"$/) do |article| features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc" features/uploads_lead.feature: And The URL of the page should contain "Nonexistent_page_abc"
On Wed, Mar 26, 2014 at 5:36 PM, Jon Robson jrobson@wikimedia.org wrote:
The URL correctness is not a good test.
When I write a test, the first check that I usually write is that the browser is at the correct page. Usually, the easiest way to check that is by checking the URL. If that causes more trouble than it is worth, then we should remove the check. If that is a valuable check, we can write a cucumber transform[1] that would make sure all page names are correctly URL encoded before comparing them with browser URL.
Željko -- 1: https://github.com/cucumber/cucumber/wiki/Step-Argument-Transforms
Out of interest, what is the value for you in writing this test?
On Thu, Mar 27, 2014 at 8:39 AM, Željko Filipin zfilipin@wikimedia.org wrote:
On Wed, Mar 26, 2014 at 5:36 PM, Jon Robson jrobson@wikimedia.org wrote:
The URL correctness is not a good test.
When I write a test, the first check that I usually write is that the browser is at the correct page. Usually, the easiest way to check that is by checking the URL. If that causes more trouble than it is worth, then we should remove the check. If that is a valuable check, we can write a cucumber transform[1] that would make sure all page names are correctly URL encoded before comparing them with browser URL.
Željko
1: https://github.com/cucumber/cucumber/wiki/Step-Argument-Transforms
On Thu, Mar 27, 2014 at 9:36 AM, Jon Robson jdlrobson@gmail.com wrote:
Out of interest, what is the value for you in writing this test?
Many of the tests for MobileFrontend were created without much review by Michelle, me, Jeff, and others. We're cleaning up a lot of cruft.
-Chris
On Thu, Mar 27, 2014 at 8:39 AM, Željko Filipin zfilipin@wikimedia.org wrote:
On Wed, Mar 26, 2014 at 5:36 PM, Jon Robson jrobson@wikimedia.org
wrote:
The URL correctness is not a good test.
When I write a test, the first check that I usually write is that the browser is at the correct page. Usually, the easiest way to check that
is by
checking the URL. If that causes more trouble than it is worth, then we should remove the check. If that is a valuable check, we can write a cucumber transform[1] that would make sure all page names are correctly
URL
encoded before comparing them with browser URL.
Željko
1: https://github.com/cucumber/cucumber/wiki/Step-Argument-Transforms
-- Jon Robson
Sure but Željko says "then I write a test, the first check that I usually write is that the browser is at the correct page" - this sounds like he is suggesting this is not cruft.... I am wondering what is the value in having this check? We have a step saying 'I am on "Foo bar" page' which we assume takes you to the correct page. Is there value in then checking the URL?
On Thu, Mar 27, 2014 at 9:40 AM, Chris McMahon cmcmahon@wikimedia.org wrote:
On Thu, Mar 27, 2014 at 9:36 AM, Jon Robson jdlrobson@gmail.com wrote:
Out of interest, what is the value for you in writing this test?
Many of the tests for MobileFrontend were created without much review by Michelle, me, Jeff, and others. We're cleaning up a lot of cruft.
-Chris
On Thu, Mar 27, 2014 at 8:39 AM, Željko Filipin zfilipin@wikimedia.org wrote:
On Wed, Mar 26, 2014 at 5:36 PM, Jon Robson jrobson@wikimedia.org wrote:
The URL correctness is not a good test.
When I write a test, the first check that I usually write is that the browser is at the correct page. Usually, the easiest way to check that is by checking the URL. If that causes more trouble than it is worth, then we should remove the check. If that is a valuable check, we can write a cucumber transform[1] that would make sure all page names are correctly URL encoded before comparing them with browser URL.
Željko
1: https://github.com/cucumber/cucumber/wiki/Step-Argument-Transforms
-- Jon Robson
On Thu, Mar 27, 2014 at 11:30 AM, Jon Robson jdlrobson@gmail.com wrote:
Sure but Željko says "then I write a test, the first check that I usually write is that the browser is at the correct page"
This is something on which Zeljko and I disagree. Especially in Mediawiki where URL schemes are highly malleable, I don't think checking for some string in a URL is very useful. URLs don't have anything to do with function, and we are creating functional tests here.
-Chris
What is the argument __for__ using them?
On Thu, Mar 27, 2014 at 11:46 AM, Chris McMahon cmcmahon@wikimedia.org wrote:
On Thu, Mar 27, 2014 at 11:30 AM, Jon Robson jdlrobson@gmail.com wrote:
Sure but Željko says "then I write a test, the first check that I usually write is that the browser is at the correct page"
This is something on which Zeljko and I disagree. Especially in Mediawiki where URL schemes are highly malleable, I don't think checking for some string in a URL is very useful. URLs don't have anything to do with function, and we are creating functional tests here.
-Chris
On Thu, Mar 27, 2014 at 5:36 PM, Jon Robson jdlrobson@gmail.com wrote:
Out of interest, what is the value for you in writing this test?
I will try to be short. If anything is unclear, please ask.
When I start testing a feature, the first test that I write is usually a check if the (correct) page opens at all. If the page does not open for what ever reason (redirect, error page, server down, network down...) all other tests will fail anyway. The experience has shown that when a lot of tests fail, it is important to quickly figure out what failed. Everything? Just a few things? Maybe just one? If the test that checks if the page opens fails, then it is clear where the problem is.
How to test if the (correct) page opens?
#1 go to the page #2 check the page
Going to the page is simple, but checking the page could take a lot of effort if we decide to check for every single page element. So, we check just the relevant ones.
My experience has shown that from all page attributes URLs tend to be the most stable ones. So, when I am thinking what should I check for first, I usually check the URL and then a few relevant page elements.
I never thought about it a lot, since URLs are pretty stable and/or easy to encode/decode. If that is not the case in your experience, feel free to delete all URL checks.
Željko
Just a thought, (and this is only if you're looking at a mediawiki page):
You can check for the correct page by looking at the body tag if that's possible. We always have a unique (page name derived) class for the body tags. (Main Page includes class 'page-Main_Page', Cucumber has 'page-Cucumber' Special:Checkuser has 'page-Special_CheckUser' etc).
Obviously this depends on the goal, is only for mediawiki pages and I don't know the testing system well enough to know if 'body has X class' is even an easy thing to check so feel free to ignore ;)
James Alexander Legal and Community Advocacy Wikimedia Foundation (415) 839-6885 x6716 @jamesofur
On Mon, Mar 31, 2014 at 6:12 AM, Željko Filipin zfilipin@wikimedia.orgwrote:
On Thu, Mar 27, 2014 at 5:36 PM, Jon Robson jdlrobson@gmail.com wrote:
Out of interest, what is the value for you in writing this test?
I will try to be short. If anything is unclear, please ask.
When I start testing a feature, the first test that I write is usually a check if the (correct) page opens at all. If the page does not open for what ever reason (redirect, error page, server down, network down...) all other tests will fail anyway. The experience has shown that when a lot of tests fail, it is important to quickly figure out what failed. Everything? Just a few things? Maybe just one? If the test that checks if the page opens fails, then it is clear where the problem is.
How to test if the (correct) page opens?
#1 go to the page #2 check the page
Going to the page is simple, but checking the page could take a lot of effort if we decide to check for every single page element. So, we check just the relevant ones.
My experience has shown that from all page attributes URLs tend to be the most stable ones. So, when I am thinking what should I check for first, I usually check the URL and then a few relevant page elements.
I never thought about it a lot, since URLs are pretty stable and/or easy to encode/decode. If that is not the case in your experience, feel free to delete all URL checks.
Željko
Mobile-l mailing list Mobile-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mobile-l
On Mon, Mar 31, 2014 at 3:19 PM, James Alexander jalexander@wikimedia.orgwrote:
Obviously this depends on the goal, is only for mediawiki pages and I don't know the testing system well enough to know if 'body has X class' is even an easy thing to check so feel free to ignore ;)
We can check (almost) anything. Looks like I to check URLs, but Chris and Jon don't. :)
Željko