Run Cloud-eligible integration tests with filtering - #1518
Conversation
086a427 to
b735380
Compare
56f8638 to
1bb491c
Compare
b735380 to
e19bfdd
Compare
1bb491c to
da76966
Compare
e19bfdd to
c7823e9
Compare
da76966 to
4fa0651
Compare
c7823e9 to
1c0f8aa
Compare
a1352d2 to
495c5e7
Compare
495c5e7 to
d5cc2f0
Compare
c16fdd3 to
519c79f
Compare
04fa067 to
4d30cdd
Compare
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
Overall looking good to me, thanks for this. Just a couple things to address. Main feedback here is we can remove a lot of the string reasons. Some of them are interesting but a lot of them have no value over the reason type.
| #[temporalio_macros::cloud_test_exclusion( | ||
| DoesNotUseServer, | ||
| "Uses MockPollCfg and a mocked SDK worker with synthetic history; no Temporal server is contacted." | ||
| )] |
There was a problem hiding this comment.
Seems like the overwhelming majority of these use a note that is 100% redundant with the categorized reason. No need for both. We should only bother with the note if it provides additional information.
| "DoesNotUseServer" | ||
| | "RequiresLocalServer" | ||
| | "RequiresCloudProvisioning" | ||
| | "NeedsCloudAdaptation" => {} |
There was a problem hiding this comment.
These reasons look like real types but aren't. These should be defined as an actual enum somewhere - it's not really possible to discover what the valid values are.
| use tracing::info; | ||
|
|
||
| #[temporalio_macros::cloud_test_exclusion( | ||
| RequiresCloudProvisioning, |
There was a problem hiding this comment.
This is more like "RequiresOSSOnlyAPIs" or something
| RequiresLocalServer, | ||
| "Requires dev-server workflow-task pagination and transaction-size dynamic configuration." |
There was a problem hiding this comment.
This is an example of a case we'll want to enable for cloud at some point once it's available there. Not sure what the best way to track that is. Maybe something formalized isn't really worth it. Open to suggestions.
2 participants
What was changed
Integration tests are now Cloud-eligible by default. Tests that do not exercise a server, require
a local server or additional Cloud provisioning, or have a confirmed Cloud incompatibility use a
source-local
#[cloud_test_exclusion(Category, "specific reason")]annotation. Cloud mode turnsthose annotations into native ignored tests, including every case generated by
rstest.The classification was built by running the suite against an isolated Cloud namespace. Direct
hard-coded
defaultnamespace assumptions were changed to use the harness-selected namespace;broader test fixes remain explicitly excluded.
Why?
Cloud compatibility should be visible where each test is maintained and based on observed
behavior, without maintaining a separate regex list. Developers can inspect skipped cases with
libtest's native
--ignored --list, while new tests enter the Cloud suite unless they declare aconcrete exclusion.
Checklist
Part of Run integration tests against cloud features#851.
How was this tested:
cargo +1.94 test -q -p temporalio-macroscargo +1.94 integ-test -s external --cloud -- --ignored --listlists all 231 exclusions.cargo +1.94 lintcargo +1.94 test-lintactionlint -ignore 'SC2086' .github/workflows/per-pr.ymlruns 33782319827 and
33785575299.
Any docs updates needed?
Yes. The README documents Cloud filtering, exclusion categories, required notes, and native
skipped-test listing.