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

New 'E2E_BROWSER' env var, and README update #5651

Merged
merged 1 commit into from Jun 3, 2020

Conversation

miyamotoh
Copy link
Contributor

Introducing a new env var to allow usage of Firefox in e2e/integration tests. See details in #4311 .

Signed-off-by: Hiro Miyamoto miyamotoh@us.ibm.com

@miyamotoh
Copy link
Contributor Author

/assign @benjaminapetersen
/assign @spadgett

@@ -15,6 +15,7 @@ import {

const tap = !!process.env.TAP;

export const BROWSER_NAME = process.env.E2E_BROWSER ? process.env.E2E_BROWSER : 'chrome';
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 prefix this with BRIDGE_ like other environment variables. (In hindsight, we should have done that for JASMINE_TIMEOUT as well.

We might want to put PROTRACTOR in the name unless we think we can support this for Cypress as well. cc @dtaylor113

Suggested change
export const BROWSER_NAME = process.env.E2E_BROWSER ? process.env.E2E_BROWSER : 'chrome';
export const BROWSER_NAME = process.env.BRIDGE_E2E_BROWSER_NAME || 'chrome';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, clearly a better syntax to do the same. And we want to pick the appropriate name. Let me know and I'll update the commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to put PROTRACTOR in the name unless we think we can support this for Cypress as well.

The Cypress test runner has a dropdown of all browsers installed on the system, so user can simply select Firefox.

Copy link
Member

Choose a reason for hiding this comment

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

@miyamotoh I'd suggest BRIDGE_PROTRACTOR_BROWSER_NAME to make it really clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Roger that, a force-push coming with the new env var and formatting changes..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, do you want JASMINE_TIMEOUT to be renamed to BRIDGE_JASMINE_TIMEOUT in this PR? I can easily do that. Let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that's a good change.

README.md Outdated
@@ -185,7 +185,7 @@ Run frontend tests:

### Integration Tests

Integration tests are run in a headless Chrome driven by [protractor](http://www.protractortest.org/#/). Requirements include Chrome, a working cluster, kubectl, and bridge itself (see building above).
Integration tests are run in a headless browser driven by [protractor](http://www.protractortest.org/#/). Requirements include Chrome or Firefox, a working cluster, kubectl, and bridge itself (see building above). By default, it will look for Chrome in the system and use it, but if you want to use Firefox instead, set `E2E_BROWSER` environment variable in your shell with the value "firefox."
Copy link
Member

Choose a reason for hiding this comment

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

Better to add line breaks. It makes comments easier in PR reviews.

I'd use backticks in the value avoid any ambiguity with the period and quotation marks.

Suggested change
Integration tests are run in a headless browser driven by [protractor](http://www.protractortest.org/#/). Requirements include Chrome or Firefox, a working cluster, kubectl, and bridge itself (see building above). By default, it will look for Chrome in the system and use it, but if you want to use Firefox instead, set `E2E_BROWSER` environment variable in your shell with the value "firefox."
Integration tests are run in a headless browser driven by [protractor](http://www.protractortest.org/#/). Requirements include Chrome or Firefox, a working cluster, kubectl, and bridge itself (see building above). By default, it will look for Chrome in the system and use it, but if you want to use Firefox instead, set `E2E_BROWSER` environment variable in your shell with the value `firefox`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sounds good. Will be in the next push, along with the new env var name.

Signed-off-by: Hiro Miyamoto <miyamotoh@us.ibm.com>
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@spadgett spadgett added this to the v4.6 milestone Jun 3, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: miyamotoh, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2020
@openshift-merge-robot openshift-merge-robot merged commit 3086656 into openshift:master Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants