Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

chore(exitCodes): adding exit code for browser connect errors #3133

Merged
merged 8 commits into from
Jun 14, 2016

Conversation

cnishina
Copy link
Member

No description provided.

@cnishina cnishina force-pushed the pr_exitCodes_directConnect branch 2 times, most recently from 8793992 to 2b528bd Compare April 16, 2016 03:45
@cnishina
Copy link
Member Author

Unit tests added for BrowserError for direct connect.

@cnishina cnishina force-pushed the pr_exitCodes_directConnect branch 2 times, most recently from 213a1df to 8b49b8a Compare April 18, 2016 21:05
@cnishina
Copy link
Member Author

depends on #3131

var fs = require('fs');
var BrowserError = require('../../../built/exitCodes').BrowserError;

var removeSeleniumFolder = function() {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing this, just use a config file that points at a different (and invalid) chromedriver location.

@cnishina cnishina force-pushed the pr_exitCodes_directConnect branch 2 times, most recently from 4dbd109 to 9def91e Compare April 19, 2016 18:41
@cnishina
Copy link
Member Author

Instead of using the selenium folder, creating a "chromedriver" file in the os.tmpdir() for my unit test.

describe('with selenium drivers', function() {
var chromedriver = '';
beforeEach(function() {
// add files to selenium folder
Copy link
Member

Choose a reason for hiding this comment

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

Why is it important that the file have the correct version? Aren't we just testing that if the file exists but isn't a proper driver, we get the right error code?

I think this should either just use a bogus file that we create (like os.tpdir() + '/foo/bar') or not specify chromeDriver in the config and let the driverProvider use its normal logic.

@cnishina cnishina force-pushed the pr_exitCodes_directConnect branch 5 times, most recently from 8ea25f0 to 602f500 Compare April 22, 2016 19:55
@cnishina cnishina changed the title chore(exitCodes): adding exit code for direct connect errors chore(exitCodes): adding exit code for browser connect errors Apr 22, 2016
@cnishina cnishina removed their assignment Apr 22, 2016
@cnishina cnishina force-pushed the pr_exitCodes_directConnect branch from 0dc0b26 to d340ab2 Compare June 8, 2016 23:15
@@ -3,10 +3,11 @@
* It is responsible for setting up the account object, tearing
* it down, and setting up the driver correctly.
*/
import * as request from 'request';
import * as https from 'https';
Copy link
Member

Choose a reason for hiding this comment

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

Can this 'request' removal be split into its own PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@juliemr
Copy link
Member

juliemr commented Jun 10, 2016

A couple clean-up comments.

* use the thrown error for stack traces instead of creating a new one
* use inheritance over creating a new error
* read in stacktrace with captureStackTrace
* removed default error messages
@@ -2,7 +2,11 @@ import {Browser, ElementHelper} from './browser';
import {ProtractorBy} from './locators';
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an unrelated change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I need to stop sneaking these in. Will revert this file back.

@juliemr
Copy link
Member

juliemr commented Jun 14, 2016

LGTM once that's done.

@cnishina cnishina merged commit 78f3c64 into angular:master Jun 14, 2016
@cnishina cnishina deleted the pr_exitCodes_directConnect branch June 14, 2016 19:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants