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

fix(cli): Allow frameworks to specify extra command line flags #3994

Merged
merged 1 commit into from
Jan 23, 2017
Merged

fix(cli): Allow frameworks to specify extra command line flags #3994

merged 1 commit into from
Jan 23, 2017

Conversation

heathkit
Copy link
Contributor

@heathkit heathkit commented Jan 21, 2017 •
edited
Loading

Fix for #3978.

Our initial plan to allow setting --disableChecks with an environment variable is insufficient, since the custom framework isn't even require()'d until after the config is parsed. This moves the unknown flag check into the runner, and gives frameworks a way to specify extra flags they accept.

cnishina
cnishina suggested changes Jan 21, 2017
View reviewed changes
lib/runner.ts
runTestPreparer(extraFlags?: string[]): q.Promise<any> {
let unknownFlags = this.config_.unknownFlags_ || [];
if (extraFlags) {
unknownFlags = unknownFlags.filter((f) => extraFlags.indexOf(f) != 0);
Copy link
Member

Choose a reason for hiding this comment

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

!== -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

cnishina
cnishina approved these changes Jan 21, 2017
View reviewed changes
lib/runner.ts
runTestPreparer(extraFlags?: string[]): q.Promise<any> {
let unknownFlags = this.config_.unknownFlags_ || [];
if (extraFlags) {
unknownFlags = unknownFlags.filter((f) => extraFlags.indexOf(f) === -1);
Copy link
Member

@cnishina cnishina Jan 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yup I stand corrected. This should be ===. Filter flags that do not exist in extra flags. LGTM.

sjelin
sjelin approved these changes Jan 23, 2017
View reviewed changes
Copy link
Contributor

@sjelin sjelin left a comment

Choose a reason for hiding this comment

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

Update the commit message to link to issue

@heathkit heathkit merged commit 5856037 into angular:master Jan 23, 2017
@heathkit heathkit deleted the disable-checks branch January 23, 2017 23:10
@heathkit heathkit mentioned this pull request Jan 23, 2017
Closed
@segrey
Copy link

segrey commented Jan 27, 2017

Hi guys!
Unfortunately, it won't help IntelliJ protractor integration which passes --intellijOriginalConfigFile=/path/to/protractor.conf. Even it already passes --disableChecks option, previously released version don't have it. What do you think about disabling checking for unknown flags in 5.1 completely, and restore this functionality in the next version? That would give us time to release IDE version with --disableChecks option. Thanks in advance!

@heathkit
Copy link
Contributor Author

Unknown flags will log a warning message in 5.1 instead of quitting with an error. We're hoping to get 5.1 out this Monday.

@segrey
Copy link

segrey commented Jan 27, 2017

Thanks for the prompt turnaround! Warning is a good response to unknown flags.

jan-molak added a commit to serenity-js/serenity-js that referenced this pull request Feb 11, 2017
@jan-molak
feat(serenity-protractor): Support for Protractor 5.1.x
a6d9eab 
This change adds support for using Serenity/JS with Protractor 5.0.0, 5.1.0 and 5.1.1:
- `ProtractorNotifier` deals with Protractor 5.1.0 requirement for test frameworks to invoke `runner.afterEach` after every test,
so that the browser is correctly restarted between the tests. See angular/protractor#4087
- `TestFrameworkDetector` registers `mochaOpts` and `cucumberOpts` as valid command line options,
benefiting from angular/protractor#3994 and dealing with a bug introduced in Protractor 5.0.0, which prevents command line arguments
from being passed on to custom test frameworks. See angular/protractor#3978
- Updated the examples and documentation to work with Protractor 5.1.x
- Enables #18

Related issues: angular/protractor#3978 angular/protractor#3994 angular/protractor#4087 #18
igniteram pushed a commit to igniteram/protractor that referenced this pull request Feb 21, 2017
@heathkit @igniteram
fix(cli): Allow frameworks to specify flags they recognize. (angular#…
e84463f 
…3994)

Fix for angular#3978.

Our initial plan to allow setting --disableChecks with an environment variable is insufficient, since the custom framework isn't even require()'d until after the config is parsed. This moves the unknown flag check into the runner, and gives frameworks a way to specify extra flags they accept.
@renovate-tests renovate-tests mentioned this pull request Jul 7, 2017
Open
@anatoliyarkhipov
Copy link

Can you please provide an example of using the unknownFlags_ option? We use protractor 5.1.2 and in the config file I tried to define custom flags:

  unknownFlags_: [
    'logX',
    '--logX',
  ],

But I still receive the warning:

$ npm run spec -- --logX

> protractor ./specs/conf.js "--logX"

[13:47:28] W/runner - Ignoring unknown extra flags: logX. This will be an error in future versions, please use
 --disableChecks flag to disable the  Protractor CLI flag checks.

Did I miss something? 🤔

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sjelin sjelin sjelin approved these changes

@cnishina cnishina cnishina approved these changes

Assignees
No one assigned
Labels
cla: yes pr: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@heathkit @segrey @anatoliyarkhipov @cnishina @sjelin @googlebot