Skip to content

Fix flaky ServerMetricsCollector integration test#65420

Merged
pgayvallet merged 2 commits intoelastic:masterfrom
pgayvallet:kbn-59236-flacky-metric-it-2
May 6, 2020
Merged

Fix flaky ServerMetricsCollector integration test#65420
pgayvallet merged 2 commits intoelastic:masterfrom
pgayvallet:kbn-59236-flacky-metric-it-2

Conversation

@pgayvallet
Copy link
Copy Markdown
Contributor

Fix #59236

@pgayvallet pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels May 6, 2020
@pgayvallet pgayvallet requested a review from a team as a code owner May 6, 2020 07:25
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-platform (Team:Platform)

expect(metrics.concurrent_connections).toEqual(0);

sendGet('/').end(() => null);
const res1 = sendGet('/').then(res => res);
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.

Supertest API... The request is sent when calling/awaiting .then. I'm forced to noop-chain the promise to execute the request without awaiting it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How it's different from .end()? I noticed that in case of end(), it logs superagent request was sent twice, because both .end() and .then() were called. Never call .end() if you use promises. Does it send a request twice 🤦

Copy link
Copy Markdown
Contributor

@mshustov mshustov May 6, 2020

Choose a reason for hiding this comment

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

Would you mind adding a comment about the usage of such a sophisticated construction in the test?

Copy link
Copy Markdown
Contributor Author

@pgayvallet pgayvallet May 6, 2020

Choose a reason for hiding this comment

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

Does it send a request twice

Yea...

const req = sendGet('/').end(() => null);
// later
await req;

Does send the request on each line... Imho having .then executing the request is a very bad decision from this lib, but that's how it works... Which is why I had to use this .then trick to be able to execute the request and still await for it later.

Will add a comment

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit c6b1f70 into elastic:master May 6, 2020
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request May 6, 2020
* fix flaky test

* add comment on supertest behavior
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 6, 2020
* master: (72 commits)
  add tsvb tests to Firefox suite (elastic#65425)
  Fix flaky ServerMetricsCollector integration test (elastic#65420)
  [APM] Custom links section inside the Actions menu is showing outside of the menu (elastic#65428)
  [ML] Adds docs_per_second to transform edit form. (elastic#65365)
  update apm index pattern (elastic#65424)
  add direct build command (elastic#65431)
  [ML] Adding daily_model_snapshot_retention_after_days to types and schemas (elastic#65417)
  [chore] Improve request cancelation handling in vis embeddable (elastic#65057)
  [Alerting] migrates acceptance and functional test fixtures to KP (elastic#64888)
  [ML] Fixes reordering in view by selection when overall cell selected (elastic#65290)
  Additional branding updates (elastic#64712)
  Remove redundant formatting of percentage column (elastic#64948)
  [SIEM][CASE] Configuration pages UI redesign (elastic#65355)
  New nav (elastic#64018)
  [Ingest pipelines] Address copy feedback (elastic#65175)
  bug fixing (elastic#65387)
  skip whole suite blocking snapshots (elastic#65377)
  add related event generation to ancestor nodes (fixes a bug) (elastic#64950)
  [Canvas] move files from legacy/plugins to plugins (elastic#65283)
  [SIEM] template timeline UI (elastic#64439)
  ...
pgayvallet added a commit that referenced this pull request May 6, 2020
* fix flaky test

* add comment on supertest behavior
@spalger spalger added the v7.9.0 label May 14, 2020
spalger pushed a commit to spalger/kibana that referenced this pull request May 14, 2020
* fix flaky test

* add comment on supertest behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.8.0 v7.9.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failing test: Jest Integration Tests.src/core/server/metrics/integration_tests - ServerMetricsCollector collect connection count

5 participants