Make side effect expectations explicit requirement.

[darshanparajuli]

Addresses #624.

Note: this is my very first PR for this project, I hope I did not make lots of mistakes (whether in assumptions or otherwise).

[CLAassistant]

All committers have signed the CLA.

[steve-the-edwards]

Thanks - looks good to me but as @rjrjr noted we'll have to plan for integration with PoS tests.

I wonder if we want to invert the API and provide opportunity to opt out of it until we have socialized the change? the Worker one was easier to socialize because it came with Workflow1 API changes.

[rjrjr]

I'm strongly inclined to YOLO this one. It's still early days for these tests, and the inconsistency w/worker is so jarring. It's hard to imagine a scenario someone has a test that's relying on breaking if a new side effect appears.

Maybe ask for discussion on the public Slack and Square Slack?

[darshanparajuli]

I have asked on both public and Square slack to see if anybody have concerns with this.

[darshanparajuli]

I don't think there were any major concerns against this, so we should be good to merge? not sure how to re-kick CI to make it happy again.

[rjrjr]

Yeah, let's ship it.

If you click through any Details link in the CI report above there is a Rerun button at the top of the next page. Clicking it now…

[darshanparajuli]

If you click through any Details link in the CI report above there is a Rerun button at the top of the next page. Clicking it now…

Thanks! I don't see "Rerun" button on the details page.

[rjrjr]

If you click through any Details link in the CI report above there is a Rerun button at the top of the next page. Clicking it now…

Thanks! I don't see "Rerun" button on the details page.

Ah, probably an ownership thing.

Anyway, I'll merge as soon as CI is green enough.

(The SDK 21 shard is so flaky that it's useless, really should remove it. But I'm hoping that the fix for #570 will tame it.)

[rjrjr]

@darshanparajuli You have ktlint violations. The best way to get your PR green is to open a Phone AVD and run ./gradlew build connectedCheck, it only takes a few minutes.

[darshanparajuli]

Ah, probably an ownership thing.

Anyway, I'll merge as soon as CI is green enough.

(The SDK 21 shard is so flaky that it's useless, really should remove it. But I'm hoping that the fix for #570 will tame it.)

Sounds good, thank you!

@darshanparajuli You have ktlint violations. The best way to get your PR green is to open a Phone AVD and run ./gradlew build connectedCheck, it only takes a few minutes.

On it.

[darshanparajuli]

CI failed with

* What went wrong:
Execution failed for task ':trace-encoder:kaptGenerateStubsKotlin'.
> Error while evaluating property 'filteredArgumentsMap' of task ':trace-encoder:kaptGenerateStubsKotlin'
   > Could not resolve all files for configuration ':trace-encoder:compileClasspath'.
      > Could not download moshi-adapters-1.12.0.jar (com.squareup.moshi:moshi-adapters:1.12.0)
         > Could not get resource 'https://repo.maven.apache.org/maven2/com/squareup/moshi/moshi-adapters/1.12.0/moshi-adapters-1.12.0.jar'.
            > Could not GET 'https://repo.maven.apache.org/maven2/com/squareup/moshi/moshi-adapters/1.12.0/moshi-adapters-1.12.0.jar'.
               > Connection reset

[rjrjr]

That's what we in the trade call "green enough," since SDK 24 passed. Merging.