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

Fix flaky ServerMetricsCollector integration test #65420

Merged
merged 2 commits into from
May 6, 2020

Conversation

pgayvallet
Copy link
Contributor

Fix #59236

@pgayvallet pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc 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
Contributor

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

sendGet('/').end(() => null);
const res1 = sendGet('/').then(res => res);
Copy link
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
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
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
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
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 Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.8.0 v7.9.0 v8.0.0
Projects
None yet
5 participants