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

[ts] migrate root test dir to project refs #99148

Merged
merged 8 commits into from
Jun 4, 2021

Conversation

spalger
Copy link
Contributor

@spalger spalger commented May 4, 2021

While working on #98337 and exploring the possibility of colocating tests with plugins I'm running into an issue because the test/ project isn't using project refs. This change wasn't simple, it required that we stop using class expressions as the return value of Service/PageObject providers.

Unfortunately this is a pattern that we use throughout the FTR providers so I'm working on PRs that migrate us away from this pattern and to a new pattern where services extend a base FtrService class, which provides a constructor that exposes the other services via this.ctx.getService('...') and this.ctx.getPageObjects([...]). See the discussion here about other possibilities we considered and their trade offs: #99148 (comment)

This approach seems to have the fewest trade offs:

  • pros
    • class is just exported from a module, which is a very TypeScript friendly move and integrates well
    • type signatures look good in editors (as opposed to POJOs which just list tons of methods as the signature)
    • class defines its own interface, no need to write separate interfaces or anything
    • using this.ctx.getService() to fetch other service handles mapping dependent types appropriately
  • cons
    • we need to use this. to reference other services which were previously available without the prefix
    • the code changes required are numerous, even if they are mostly mundane

To help educate people of the change and to make review of this pretty massive API change more manageable I broke up the services into many smaller PRs which specific teams reviewed. This PR now includes all the PageObjects and a handful of remaining services in one final step as the conflicts caused by breaking the PR up were really hard to deal with and I think info about this change is starting to spread naturally.

Reviewers note

The vast majority of changes are just extracting classes from providers and remapping access to services/pageObjects to properties on this. Viewing the changes side-by-side and without whitespace will help you see the changes in this PR for just what they are.

This was referenced May 25, 2021
@dmlemeshko
Copy link
Member

One of the PRs failed with the error:

Error: retry.try timeout: TypeError: Cannot read property 'testSubjects' of undefined
    at getFn (test/functional/services/inspector.ts:230:17)
    at block (test/functional/apps/visualize/_vega_chart.ts:126:49)
    at runAttempt (test/common/services/retry/retry_for_success.ts:27:21)
    at retryForSuccess (test/common/services/retry/retry_for_success.ts:66:27)
    at RetryService.try (test/common/services/retry/retry.ts:31:12)
    at Context.<anonymous> (test/functional/apps/visualize/_vega_chart.ts:125:13)
    at Object.apply (node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)
    at onFailure (test/common/services/retry/retry_for_success.ts:17:9)
    at retryForSuccess (test/common/services/retry/retry_for_success.ts:57:13)
    at RetryService.try (test/common/services/retry/retry.ts:31:12)
    at Context.<anonymous> (test/functional/apps/visualize/_vega_chart.ts:125:13)
    at Object.apply (node_modules/@kbn/test/src/functional_test_runner/lib/mocha/wrap_function.js:73:16)

@spalger The fix was trivial 152bdda, but I wonder if TS can help us avoid similar usage in future?

@spalger
Copy link
Contributor Author

spalger commented May 26, 2021

@spalger The fix was trivial 152bdda, but I wonder if TS can help us avoid similar usage in future?

Unfortunately I don't think there is anything that TS can do to help us here. this context is tricky, there isn't any way to require that class methods be called with the correct this context, but at least it's stable so those tests would never pass or make it out of a PR, so I'm not super worried about it.

@spalger spalger force-pushed the implement/root-test-ts-project branch 2 times, most recently from c9ec614 to b254db2 Compare June 2, 2021 02:28
@kibanamachine

This comment has been minimized.

@spalger spalger force-pushed the implement/root-test-ts-project branch from 2e88357 to 2dcb164 Compare June 2, 2021 14:22
@spalger spalger marked this pull request as ready for review June 2, 2021 14:27
@spalger spalger requested a review from a team as a code owner June 2, 2021 14:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-qa (Team:QA)

@kibanamachine

This comment has been minimized.

Copy link

@LeeDr LeeDr left a comment

Choose a reason for hiding this comment

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

LGTM - code reviewed only.

test/functional/page_objects/common_page.ts Outdated Show resolved Hide resolved
@spalger spalger requested a review from a team as a code owner June 4, 2021 15:32
@spalger spalger enabled auto-merge (squash) June 4, 2021 16:37
Copy link
Contributor

@brianseeders brianseeders left a comment

Choose a reason for hiding this comment

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

LGTM operations team changes

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@spalger spalger merged commit 090d0ab into elastic:master Jun 4, 2021
@spalger spalger deleted the implement/root-test-ts-project branch June 4, 2021 17:17
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 99148

spalger added a commit to spalger/kibana that referenced this pull request Jun 4, 2021
Co-authored-by: spalger <spalger@users.noreply.github.com>
spalger added a commit that referenced this pull request Jun 4, 2021
* [ts] migrate root test dir to project refs (#99148)

Co-authored-by: spalger <spalger@users.noreply.github.com>

* include schema files in telemetry ts project

Co-authored-by: spalger <spalger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team Team:QA Team label for QA Team v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants