Skip to content

Removes filtering out of client nodes from bad nodes list#6840

Merged
ycombinator merged 4 commits intoelastic:masterfrom
ycombinator:gh-5732
Apr 12, 2016
Merged

Removes filtering out of client nodes from bad nodes list#6840
ycombinator merged 4 commits intoelastic:masterfrom
ycombinator:gh-5732

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

Resolves #5732

return checkEsVersion(server)
.then(function() {
throw new Error('expected validation to fail')
}, _.noop);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm. Looks like the example copied of a fail-check could be improved. It seems like it works because of an unhandled error, and the test is made to fail if there's no error that slips by.

A more typical way to test that a function throws an error is something like:

function checkWithBadClientNode() {
  return checkEsVersion(server);
}
expect(checkWithBadClientNode).to.throwException(function (e) { // get the exception object
  expect(e).to.be.a(SetupError);
});

That kind of test will be possible if you import expect and SetupError into the suite. I prefer this way because it is more explicit what kind of error gets thrown for the execution path you want to test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @tsullivan, I tried it the way you suggested but it didn't work. So I had Spencer look over my work and he made me realize why: checkEsVersion is asynchronous and returns a Promise so the proposed assertion won't work as expected.

I do, however, agree that we can make a stronger assertion using SetupError here than the one I'm currently making. I'll update this test shortly.

@tsullivan tsullivan assigned ycombinator and unassigned tsullivan Apr 11, 2016
@ycombinator ycombinator assigned tsullivan and unassigned ycombinator Apr 11, 2016
@tsullivan
Copy link
Copy Markdown
Member

LGTM

@tsullivan tsullivan removed the review label Apr 12, 2016
@tsullivan tsullivan assigned ycombinator and unassigned tsullivan Apr 12, 2016
@ycombinator ycombinator merged commit 8624914 into elastic:master Apr 12, 2016
@ycombinator ycombinator deleted the gh-5732 branch April 12, 2016 15:57
breehall added a commit that referenced this pull request Jun 14, 2023
eui@81.3.0 ⏩ eui@82.1.0

## [`82.1.0`](https://github.com/elastic/eui/tree/v82.1.0)

- Added ability for `EuiMarkdownEditor` plugins to disable toolbar
buttons ([#6840](elastic/eui#6840))

## [`82.0.0`](https://github.com/elastic/eui/tree/v82.0.0)

**Bug fixes**

- Fixed `EuiPopover`'s types to omit `panelProps.hasBorder` and
`panelProps.hasShadow` - these props are not customizable on popovers
for visual consistency
([#6836](elastic/eui#6836))

**Breaking changes**

- `EuiRange` & `EuiDualRange` no longer have a hard limit of 20
displayed ticks. The component now instead detects the width available,
and throws an error if each tick has less than 5 pixels of width. We
recommend testing your tick usage at smaller screens to ensure they
always display legibly to users.
([#6829](elastic/eui#6829))
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