Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added test for zh_TW locale #10

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

rbillings
Copy link

FIx for issue #5

@rbillings
Copy link
Author

r+?

@bobsilverberg
Copy link
Contributor

@rbillings Hmm, I'm not sure why this is in the marketing-project-template repo. Should this be in mcom-tests or some other repo? This repo isn't meant for actual tests. It's just a template that can be used by folks who are looking to create a new test suite.

@rbillings
Copy link
Author

@stephendonner - is there a reason why #5 is in this repo?

@rbillings
Copy link
Author

@stephendonner ping

@stephendonner
Copy link
Contributor

Sorry I missed the earlier ping.

  1. running as "engagement.template," this was actually used (primarily in the Firefox 4 days) for Marketing/Engagement test-automation -- and it found bugs like missing favicons, bad links, etc. zh-TW should *never* fall-back to zh-CN #5 was specific to Firefox Flicks, which was the latest/last project to really have this suite run against it
  2. I still think there's value in some of these tests - both as potential stand-alones in this suite, run against a new site, or copied over in one of our example test-automation repos (e.g. @davehunt 's new one).

Here's the actual locale test that issue #5 was hoping to agument: https://github.com/mozilla/marketing-project-template/blob/master/tests/test_file.py#L32

@@ -45,6 +45,18 @@ def test_locale(self, mozwebqa):
print "There is no language selector on the page"

@pytest.mark.nondestructive
def test_locale_zh_TW (self, mozwebqa):
main_page = MySiteHomePage(mozwebqa)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include a docstring to explain the test here. If I understand correctly we want to check that opening [base]/zh_TW should either stay as [base]/zh_TW or become [base]/en-US. If this is the case I would attempt to make this test a little more generic using a list of locales that this could apply to (with just one for now). I would therefore also update the name of the test to reflect this. Something like test_locales_persist_or_fallback_to_en_us.

@davehunt
Copy link
Member

I think @bobsilverberg was confusing this with the https://github.com/mozilla/mozwebqa-test-templates repository. Even though this suite is not currently being run (since the Jenkins migration) the issue/test seems valid to me.

@bobsilverberg
Copy link
Contributor

I'm just reviewing all open PRs and found this one. I must admit I'm still a bit confused about it, and yes, I guess I was confusing this repo with https://github.com/mozilla/mozwebqa-test-templates.

In any case, there have been a bunch of comments made my @davehunt that are waiting to be addressed. @rbillings are you going to continue with this PR and attempt to address @davehunt's comments?

@rbillings
Copy link
Author

I will complete this- it's just low on the totem pole. I'll aim for early June as I doubt I'll get to it this week.

@rbillings
Copy link
Author

I updated this, but am unsure how to run the test to verify that it would actually work.

@@ -9,6 +9,7 @@

from pages.page_object import MySiteHomePage
from unittestzero import Assert
from urlparse import urljoin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Standard library imports should be at the top. See http://legacy.python.org/dev/peps/pep-0008/#imports

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants