-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Ingest Manager] Add more Fleet concurrency tests #71744 #72338
[Ingest Manager] Add more Fleet concurrency tests #71744 #72338
Conversation
@elasticmachine merge upstream |
Pinging @elastic/ingest-management (Team:Ingest Management) |
@elastic/kibana-platform Can someone take a look at the two TS/JS issues I mention in the description? Let me know if I can give more information or pointers for any changes I should make to my code or platform code to fix/avoid them. |
@elasticmachine merge upstream |
1/ mockPreAuthToolkit return type We mixed something up when we added the kibana/src/core/server/http/http_service.mock.ts Lines 178 to 181 in 0ea414c
2/ TypeError: Invalid event target for request.events.aborted$ I confirm I can reproduce the error import { httpServerMock } from './http_server.mocks';
describe('foo', () => {
it('bar', async () => {
const req = httpServerMock.createKibanaRequest();
await req.events.aborted$.toPromise();
});
}); causes TypeError: Invalid event target
at setupSubscription (/Users/pierregayvallet/DEV/workspaces/elastic/kibana/node_modules/rxjs/src/internal/observable/fromEvent.ts:229:11)
at Observable._subscribe (/Users/pierregayvallet/DEV/workspaces/elastic/kibana/node_modules/rxjs/src/internal/observable/fromEvent.ts:204:5)
at Observable.Object.<anonymous>.Observable._trySubscribe (/Users/pierregayvallet/DEV/workspaces/elastic/kibana/node_modules/rxjs/src/internal/Observable.ts:238:19) 3/ 2) seems to prevent decrease() from being called in the tests, but it is called in the app code. Fixing the mock implementation to no longer throw is quite easy, however you have to keep in mind that the cc @restrry |
Created #72663 |
@jen-huang wdyt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test suite looks good based on the above limitations that we have.
Fixing the mock implementation to no longer throw is quite easy, however you have to keep in mind that the req.events.aborted$ observable will never emit from a mocked request in a jest unit test environment, meaning that decrease will not be called anyway.
@pgayvallet How can we prevent TypeError: Invalid event target
from being thrown? @jfsiii I'd like to fix this aspect before merging, as the current error causes the test suite output to be quite noisy.
}); | ||
|
||
const mockRequest = httpServerMock.createKibanaRequest({ | ||
path: '/no/match', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the description, this looks like it should have a non-matching routeTags
specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could have tags which do not match or, like this case, no tags at all. I could add some
@jen-huang Isn't it fixed in #72663 ? |
It is, and the PR has just been merged. Could you please merge master and confirm both issues are resolved? |
@jen-huang since the doesn't require any changes to the test code I'd like to merge now |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood where the work needed to be done for the error not to be thrown :)
LGTM. Thanks for adding these tests!
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
* master: (35 commits) Migrated karma tests to jest (elastic#72649) Migrate status page app to core (elastic#72017) Failing test: Jest Tests.src/plugins/vis_type_vega/public (elastic#71834) Fix Firefox TSVB flaky test with switch index patterns (elastic#72882) [ML] Fixing link to index management from file data visualizer (elastic#72863) test: 💍 add test for sub-expression variables (elastic#71644) fix bug (elastic#72809) [keystore] use get_keystore in server cli (elastic#72954) Show step number instead of incomplete step. (elastic#72866) Fix bug where user can't add an exception when "close alert" is checked (elastic#72919) [Monitoring] Fix issues displaying alerts (elastic#72891) [Ingest Manager] Add more Fleet concurrency tests elastic#71744 (elastic#72338) [Security Solution][Exceptions] - Update UI exceptions builder nested logic (elastic#72490) disable renovate masterIssue [ML] API integration tests for UPDATE data frame analytics endpoint (elastic#72710) [Uptime] Fix accessibility issue in Uptime app nav links (elastic#72926) [Maps] fix removing global filter from layer can cause app to start thrashing (elastic#72763) [Maps] fix blended layer aggregation error when using composite aggregation (elastic#72759) fix unexpected arguments to unload command Limits the upload size of lists to 9 meg size (elastic#72898) ...
Don't forget backport :) As this is adding more test coverage (rather than fixing an existing test that impacts other branches, etc), I updated the label to just 7.10 instead of all the way back to 7.9. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync. |
…3213) * Refactor to make more testable. Add more tests. * Remove ts-ignores. Add comment re: testing limitation Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
refs #71744
The limited concurrency is enforced by a global preAuth interceptor.
limited_concurrency.ts
exportscreateLimitedPreAuthHandler
which allows to configure the "middleware" then make calls to it with mock data;Looking for 👀& feedback on the approach & two JS/TS issues:
(1) see
// @ts-ignore error re: mockPreAuthToolkit return type
comments(2)
TypeError: Invalid event target
forrequest.events.aborted$
(2) seems to prevent
decrease()
from being called in the tests, but it is called in the app code.Checklist