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

[feature] Adding urlIs and urlContains to expectedConditions class #3237

Merged
merged 3 commits into from
Jun 3, 2016
Merged

[feature] Adding urlIs and urlContains to expectedConditions class #3237

merged 3 commits into from
Jun 3, 2016

Conversation

manoj9788
Copy link
Contributor

Added a new feature UrlIs and UrlContains. Tests have passed and all check is done from my side.
happy to change if anything is needed.

manoj9788 added 2 commits May 22, 2016 21:39
declared as a var baseUrlFromSpec - to prevent tests failure, just incase if anyone builds and runs protractor test app with different port number
@juliemr
Copy link
Member

juliemr commented Jun 1, 2016

This looks good to me. We should re-think how Expected Conditions and webdriver's until work together and unify them at somepoint, but that's out of scope here.

@cnishina do you mind merging?

@manoj9788
Copy link
Contributor Author

@juliemr I do some contributions to until class of selenium too along with jleyba help.
you can involve me if required for any discussion on that.


it('should have urlIs', function() {
var baseUrlFromSpec = browser.baseUrl;
expect(EC.urlIs('http://localhost:8081').call()).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Instead of using this url, we should use the one provided from the environment variables:

var env = require('../environment');
expect(EC.urlIs(env.baseUrl).call()).toBe(false);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we waiting for @juliemr to comment or Should I push in the commit ?

Copy link
Member

Choose a reason for hiding this comment

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

@cnishina is right, good catch.

Copy link
Contributor Author

@manoj9788 manoj9788 Jun 3, 2016

Choose a reason for hiding this comment

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

@juliemr @cnishina
baseUrl: env.baseUrl + '/ng1/', is what I am seeing in the conf file. Its same as what Craig have suggested. I get that appended with /ng1/ its makes tests look neat.
Refer: https://github.com/angular/protractor/blob/master/spec/basicConf.js#L21

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the baseUrlFromSpec is fine too. It just should not have the url as localhost. @manoj9788 please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cnishina Just realized what you meant. Fixed the hardcoded url with env.baseUrl

@cnishina cnishina merged commit 8316917 into angular:master Jun 3, 2016
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