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

Refactor integration tests #1299

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Refactor integration tests #1299

merged 3 commits into from
Nov 22, 2023

Conversation

dandavison
Copy link
Contributor

This is pure refactoring of the integration tests, done in preparation for implementation of Workflow Update in #1277.

@dandavison dandavison requested a review from a team as a code owner November 21, 2023 01:36
@dandavison dandavison force-pushed the refactor-integration-tests branch from 80f98fa to f73bf7f Compare November 21, 2023 01:43
@bergundy
Copy link
Member

Please fix the lint warnings before merging.

mjameswh
mjameswh reviewed Nov 21, 2023
View reviewed changes
packages/test/src/integration-tests-old.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add a comment early in this file that explains how these tests are "old" (ie. they are still valid and up-to-date, but need to be converted to the new testing approach).

mjameswh
mjameswh reviewed Nov 21, 2023
View reviewed changes
packages/test/src/integration-tests/helpers.ts
} from '@temporalio/worker';
import * as workflow from '@temporalio/workflow';
import { ConnectionInjectorInterceptor } from '../activities/interceptors';
import { Worker, test as anyTest, bundlerOptions } from '../helpers';
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be difficult/disruptive to merge the content of this file into ../helpers.ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good, I'm happy for any further renaming/reorganizing tweaks to be done here; I just wanted to start getting things in shape while adding new tests for Update.

mjameswh
mjameswh approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@mjameswh mjameswh left a comment

Choose a reason for hiding this comment

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

I'm just a bit uncomfortable with file names (-old and helpers). I gave some details inline.

Not blocking however. Feel free to merge as is if you prefer. These can be addressed at a later point.

@dandavison
Copy link
Contributor Author

Thanks for reviewing. The file name in this PR I am most uncomfortable with is test-integration-workflows.ts => integration-tests/test-misc.ts! Please rename that if you can think of a better name. But I think the -old suffix is helpful. My objectives with this PR were to eliminate duplication of helper boilerplate, make it easy to discern which are our new-style integration tests, and which our old, while allowing the new-style integration tests to be spread over multiple files (e.g. for the coming update tests, which are in a few different files due to the use of difference interceptors). We don't have to keep the new-style ones in a directory; we could have test-integration-*.ts and test-integration-old-*.ts.

@dandavison dandavison merged commit 324db87 into temporalio:main Nov 22, 2023
23 checks passed
@dandavison dandavison deleted the refactor-integration-tests branch November 22, 2023 02:32
@dandavison dandavison restored the refactor-integration-tests branch November 22, 2023 02:32
@dandavison dandavison mentioned this pull request Nov 23, 2023
Merged
antlai-temporal pushed a commit to antlai-temporal/sdk-typescript that referenced this pull request Nov 29, 2023
@dandavison
Flatten test directory structure (temporalio#1303)
7bd36df 
There was a mistake in
temporalio#1299: tests inside the
`integration-tests` subdirectory were not being run.

But that mistake emphasizes that it's not really desirable to have the
nested directory. So instead of modifying the `test` and `test.watch`
command definitions in `package.json`, this PR flattens the directory
structure as follows

```
helpers.ts
helpers-integration.ts     // helper boilerplate shared by the new-style integration tests 
test-integration-*.ts      // i.e. new style integration tests (i.e. not matching -old-*.ts)
test-integration-old-*.ts  // old-style integration tests
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjameswh mjameswh mjameswh approved these changes

@bergundy bergundy bergundy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dandavison @bergundy @mjameswh