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

Bug 1132709 - Convert Mozmill testUnknownIssuer #96

Merged
merged 1 commit into from Feb 25, 2015

Conversation

Projects
None yet
3 participants
@galgeek
Copy link
Collaborator

galgeek commented Feb 20, 2015

def setUp(self):
FirefoxTestCase.setUp(self)

self.test_data = 'https://ssl-unknownissuer.mozqa.com'

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

To be consistent with currently landed tests for Marionette please name this self.url.


def tearDown(self):
try:
self.windows.close_all([self.browser])

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

This test is not opening new windows. So there is no need to call this method.

self.assertRaises(MarionetteException, self.marionette.navigate, self.test_data)

# Wait for about:error page
time.sleep(1)

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

This comment should be Wait for the DOM to receive events. Maybe @chmanchester could also have a look at this given that we might want to have this in Marionette proper?

# Check the issuer in cert_domain_link
issuer = (self.marionette.find_element(By.ID, 'cert_domain_link')).get_attribute(
'textContent')
self.assertEquals(issuer, 'ssl-selfsigned-unknownissuer.mozqa.com')

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

Please separate the retrieval of elements and working with those. Here I would use the get_attribute call inside assertEquals.

# Verify the error code is correct
text = (self.marionette.find_element(By.ID, 'technicalContentText')).get_attribute(
'textContent')
self.assertTrue('sec_error_unknown_issuer' in text)

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

Same as mentioned above. Also there is assertIn, which you can use here.

FirefoxTestCase.tearDown(self)

def test_unknown_issuer(self):
self.marionette.set_context('content')

This comment has been minimized.

@whimboo

whimboo Feb 20, 2015

Contributor

Personally I would do that in setUp if the whole test works in content scope. Reason is that it is part of the setup logic.

@galgeek galgeek self-assigned this Feb 20, 2015


self.url = 'https://ssl-unknownissuer.mozqa.com'

self.marionette.set_context('content')

This comment has been minimized.

@whimboo

whimboo Feb 23, 2015

Contributor

I'm a bit worried here. Do we ever reset the context to chrome? Right now we should fail in tearDown if something is broken here. I feel you should better use the with expression in the test method.

time.sleep(1)

# Check the issuer in cert_domain_link
issuer = self.marionette.find_element(By.ID, 'cert_domain_link')

This comment has been minimized.

@whimboo

whimboo Feb 23, 2015

Contributor

It's the domain and not the issuer.

@galgeek galgeek force-pushed the galgeek:unknownIssuer branch 3 times, most recently from 7fe5668 to 8d06b77 Feb 23, 2015


# Verify the "Get Me Out Of Here!" and "Add Exception" buttons appear
self.assertIsNotNone(self.marionette.find_element(By.ID, 'getMeOutOfHereButton'))
self.assertIsNotNone(self.marionette.find_element(By.ID, 'exceptionDialogButton'))

This comment has been minimized.

@whimboo

whimboo Feb 24, 2015

Contributor

I think it may be better to check with is_displayed but that's more an enhancement, which we even didn't do in the Mozmill test. You can file a followup bug as a good first bug.

@whimboo

This comment has been minimized.

Copy link
Contributor

whimboo commented Feb 24, 2015

Please rebase this PR to master and squash all the commits. Once done I can get it merged.

@galgeek galgeek force-pushed the galgeek:unknownIssuer branch from 8d06b77 to ab89a5f Feb 24, 2015

@galgeek

This comment has been minimized.

Copy link
Collaborator Author

galgeek commented Feb 24, 2015

Hi, @whimboo!

This PR is ready to merge.

Followup bug filed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1136237

@chmanchester chmanchester merged commit ab89a5f into mozilla:master Feb 25, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment