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

improved collection of failed specs #1

Closed
wants to merge 1 commit into from

Conversation

odaems
Copy link

@odaems odaems commented Aug 18, 2015

regex now matches lazy (using '?' after '*' in group) instead of greedy avoiding the need for result slicing and potential problems with multidigit line numbers
failed specs only get added once (especially important when using multiCapabilities)

regex now matches lazy (using '?' after '*' in group) instead of greedy avoiding the need for result slicing and potential problems with multidigit line numbers
failed specs only get added once (especially important when using multiCapabilities)

export default function (output = '') {
// this could all probably fit into one regex...
var failedSpecLines = output.match(FAILED_LINE);
var output = arguments.length <= 0 || arguments[0] === undefined ? '' : arguments[0];
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a need for this? output should always just be a giant blob of text (although perhaps it is different for multi capabilities?)

Copy link
Author

Choose a reason for hiding this comment

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

This was actually copy&pasted from the code that npm install protractor-flake delivered. Otherwise I agree that it's not necessary.

@NickTomlin
Copy link
Owner

This is great! Would it be possible to include some additional test coverage that parses multiCapabilities fixture text?

@@ -1,13 +1,15 @@
const FAILED_LINE = /at \[object Object\]\.<anonymous> \((.*)\)/g;
const FAILED_LINES = /at \[object Object\]\.<anonymous> \((.*?):.*\)/g;
Copy link
Owner

Choose a reason for hiding this comment

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

We should define this as a non-constant within the body of the function, since exec modifies the state of the regex it is invoked on.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed.

@NickTomlin
Copy link
Owner

I did a draft of a version that uses set. What do you think?

export default function (output = '') {
  let match = null;
  let FAILED_LINES = /at \[object Object\]\.<anonymous> \((.*?):.*\)/g;
  let failedSpecs = new Set();

  while (match = FAILED_LINES.exec(output)) {
    failedSpecs.add(match[1]);
  }

  return [...failedSpecs];
}

@odaems
Copy link
Author

odaems commented Aug 19, 2015

looks good to me as long as Set works as intended.

@NickTomlin
Copy link
Owner

I went ahead and merged this change in (with the Set implementation and some tests). @odaems thanks so much for finding and fixing this! I'll get a new release out on NPM in a few hours.

@odaems
Copy link
Author

odaems commented Aug 21, 2015

You're welcome! Thanks for making this available in the first place!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants