Add WordPress & WordPressProcessors & ImageEditor tests to Buildkite #14918
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
| - label: "Test WordPress" | ||
| <<: *docker-container | ||
| command: | | ||
| cp gradle.properties-example gradle.properties |
There was a problem hiding this comment.
The reason we repeat this cp command on every step is because they all run in parallel and in a dedicated container and there's no explicit checkout because Buildkite does it automatically.
I haven't reviewed many Buildkite PRs so far and it's been a while since I worked with it. Can you confirm that my understanding is correct, please? 😄
There was a problem hiding this comment.
Yes, checkout is handled by Buildkite and every job currently runs in parallel, meaning separate VMs and Docker containers, so they all need to copy gradle properties. I will propose dropping gradle.properties completely in an internal RFC soon, so hopefully we'll get rid of these steps.
I haven't reviewed many Buildkite PRs so far and it's been a while since I worked with it.
This is the second Buildkite PR, so you haven't missed much :)
| <<: *docker-container | ||
| command: | | ||
| cp gradle.properties-example gradle.properties | ||
| ./gradlew testWordpressVanillaRelease |
There was a problem hiding this comment.
The call on develop is different
WordPress-Android/.circleci/config.yml
Line 58 in a863317
Why did you decide to drop the --stacktrace flag in all the steps and the --no-daemon here?
There was a problem hiding this comment.
re: --stacktrace; I find stacktrace to be useful in very rare cases and it just adds noise otherwise. I am willing to bet that is the case for most people and would prefer to add it if it was requested/necessary.
re: --no-daemon; Gradle officially recommends all builds to use the daemon, and that includes CI. I am guessing this flag was passed to address an issue in the past, or it used to be recommended not to use the daemon in CI so maybe that advice was being followed, either way I don't think it's correct to use it right now. If daemon is failing, it's more likely that there is a problem on our end that should be fixed, but thankfully that's not the case right now.
There was a problem hiding this comment.
According to a quick git pickaxe, this --no-daemon was:
- introduced by @hypest in c4c9f8d
- removed and introduced back by @JavonDavis in Update App store screenshots #13211 / 399e787 .
Maybe they can provide more context as to why this was added at that time?
#11833 seems to provide a lot of context about this too, which makes me tend to agree that this was something that might have been needed in the past as a workaround but can now safely be removed.
There was a problem hiding this comment.
Thanks for the extra context @AliSoftware @oguzkocer 🙌
There was a problem hiding this comment.
Maybe they can provide more context as to why this was added at that time?
👋 friends, Olivier already shared the commits/PRs that are relevant and tried to capture the context there but, let me know if there's a specific question or unclear thing at this stage.
Here is the CircleCI counterpart which will be removed once this PR is merged.
Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
What automated tests I added (or what prevented me from doing so)
N/A
PR submission checklist:
RELEASE-NOTES.txtif necessary.