Skip to content
This repository has been archived by the owner on Aug 20, 2018. It is now read-only.

Update logic for is_the_current_page to address titles on dev #91

Merged
merged 2 commits into from
Jan 3, 2013

Conversation

bobsilverberg
Copy link
Contributor

This addresses a number of failures on moztrap.dev. It looks like the page titles have changed to be labelled Moztrap-Dev, instead of just Moztrap. Simply changing the logic to use contains instead of equals should address this, without losing the safety of the check.

Note there are other failures on dev which still exist after applying this change, which I intend to address with another pull request.

@klrmn
Copy link
Contributor

klrmn commented Jan 2, 2013

searching for _page_title in this repo leads me to discover there are 3 possible page titles in moztrap:
'Moztrap', 'BrowserID' (from the browserid subproject), and 'Login | MozTrap' (used once). I think that checking the page title for this project is kinda bogus, and doesn't prove anything about what page you're on. on the other hand, this might be a feature request that Moztrap use useful page titles.

@camd
Copy link
Contributor

camd commented Jan 2, 2013

Funny you should mention this. I just implemented useful page titles in moztrap in 1.3.7 (going on Staging today, likely). Hopefully they're useful, at least. You can check them out on moztrap-dev now, if you like.

@klrmn
Copy link
Contributor

klrmn commented Jan 2, 2013

laugh. yes, that would solve it =)

@bobsilverberg
Copy link
Contributor Author

This is good news because when I was looking at the code for is_the_current_page I thought that it could use a refactor. The code in the page objects contains assertions which I'm not really keen on, although I'm not sure if that can be avoided entirely due to the nature of how the page objects are used. Still, I think it is worth looking at more closely, and that can be done along with an attempt to use the new useful page titles.

I wanted to get the suite passing again, and I still think this pull is worthwhile for that as the is_the_current_page checks will not be any less useful with this change.

I will open an issue to address the refactor and using new page titles.

@bobsilverberg
Copy link
Contributor Author

I have updated the page objects to use the new titles. The tests are passing on dev but not yet on staging.

@stephendonner
Copy link
Contributor

Thx, Bob; I see the run passing in Jenkins, and the context + comments make this a clear win. r+, merging in.

stephendonner added a commit that referenced this pull request Jan 3, 2013
Update logic for is_the_current_page to address titles on dev
@stephendonner stephendonner merged commit 510ac0d into mozilla:master Jan 3, 2013
@bobsilverberg bobsilverberg deleted the title_fix branch January 3, 2013 17:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants