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.

Sign up for GitHub

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

Jump to bottom

[APM] Convert unit tests using mockApmEventClient to API tests #152557

Open
sorenlouv opened this issue Mar 2, 2023 · 4 comments
Open

[APM] Convert unit tests using mockApmEventClient to API tests #152557

sorenlouv opened this issue Mar 2, 2023 · 4 comments
Labels
apm:needs-test apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 Team:APM All issues that need APM UI Team support

Comments

@sorenlouv
Copy link
Member

sorenlouv commented Mar 2, 2023 •
edited by achyutjhunjhunwala
Loading

In the past we didn't have API test so on the server we'd mock out Elasticsearch using mockApmEventClient. This way we could at test the data transformation happening after the ES call. The downside of this is of course that any changes to the actual ES query will go completely unnoticed.
Today we have a solid API test suite so we should convert unit tests that mocks out the ES request. This is easy to find. Simply search for mockApmEventClient. The only exception I found was lib/helpers/transactions/get_is_using_transaction_events.test.ts which might be a valid use case to keep as a unit test

The following unit tests could be converted to API tests:

  • server/routes/environments/get_all_environments.test.ts
  • server/routes/environments/get_environments.test.ts
  • server/routes/errors/distribution/queries.test.ts
  • server/routes/metrics/queries.test.ts
  • server/routes/services/queries.test.ts
  • server/routes/services/annotations/get_derived_service_annotations.test.ts
  • server/routes/settings/agent_configuration/queries.test.ts
  • server/routes/settings/custom_link/get_transaction.test.ts
  • server/routes/storage_explorer/is_cross_cluster_search.test.ts
  • server/routes/traces/queries.test.ts
  • server/routes/transactions/queries.test.ts
  • server/routes/transactions/breakdown/index.test.ts
  • server/routes/metrics/by_agent/java/gc/fetch_and_transform_gc_metrics.test.ts
@sorenlouv sorenlouv added the Team:APM All issues that need APM UI Team support label Mar 2, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:APM)

@sorenlouv sorenlouv added apm:needs-test apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 labels Mar 2, 2023
@achyutjhunjhunwala
Copy link
Contributor

We can simply remove the Unit Test for

  • server/routes/environments/get_all_environments.test.ts
  • server/routes/environments/get_environments.test.ts

The API tests for these were added as part of this PR

@sorenlouv
Copy link
Member Author

sorenlouv commented Mar 2, 2023 •
edited
Loading

@achyutjhunjhunwala Sounds good! Will you remove and update this issue? Thanks!

@achyutjhunjhunwala achyutjhunjhunwala mentioned this issue Mar 2, 2023
Merged
@achyutjhunjhunwala
Copy link
Contributor

After further analysing, i can confirm that we can remove the Unit Tests for

  • server/routes/environments/get_environments.test.ts

PR can be found here.

Regarding the Units Test for server/routes/environments/get_all_environments.test.ts, this function is used by Agent Configuration ('GET /api/apm/settings/agent-configuration/environments') and Anomaly Detection ('GET /internal/apm/settings/anomaly-detection/environments') routes and i see both of them are missing API tests. Hence i will keep this route in this list

achyutjhunjhunwala added a commit that referenced this issue Mar 7, 2023
@achyutjhunjhunwala @kibanamachine
[APM] remove redundant unit tests for getEnvironment (#152577)
bc64e05 
## Summary

Removes redundant Unit Tests and Snapshots.

As part of the list mentioned here
#152557 (comment),
the getEnvironment logic now has its own API tests and hence we can
remove these unit tests.

API tests were added as part of this
[PR](https://github.com/elastic/kibana/pull/150175/files#diff-00aefdb700a89f09360583a1a951083746eb2243e0607de9ab6bc81826a7adaf)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this issue Mar 10, 2023
@achyutjhunjhunwala @kibanamachine @bmorelli25
[APM] remove redundant unit tests for getEnvironment (elastic#152577)
190f01c 
## Summary

Removes redundant Unit Tests and Snapshots.

As part of the list mentioned here
elastic#152557 (comment),
the getEnvironment logic now has its own API tests and hence we can
remove these unit tests.

API tests were added as part of this
[PR](https://github.com/elastic/kibana/pull/150175/files#diff-00aefdb700a89f09360583a1a951083746eb2243e0607de9ab6bc81826a7adaf)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@sorenlouv sorenlouv mentioned this issue Apr 25, 2023
Closed
@gbamparop gbamparop mentioned this issue Jun 19, 2023
Closed
@sorenlouv sorenlouv mentioned this issue Oct 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
apm:needs-test apm:test-plan-8.8.0 APM UI Test Plan for v8.8.0 Team:APM All issues that need APM UI Team support
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@sorenlouv @achyutjhunjhunwala @elasticmachine