Skip to content

Add WordPress & WordPressProcessors & ImageEditor tests to Buildkite #14918

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

Merged
oguzkocer merged 1 commit into from
Jun 24, 2021
Merged

Add WordPress & WordPressProcessors & ImageEditor tests to Buildkite #14918

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,18 @@ steps:
command: |
cp gradle.properties-example gradle.properties
./gradlew lintWordpressVanillaRelease
- label: "Test WordPress"
<<: *docker-container
command: |
cp gradle.properties-example gradle.properties
Comment on lines +29 to +32

@mokagio mokagio Jun 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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? 😄

@oguzkocer oguzkocer Jun 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 :)

./gradlew testWordpressVanillaRelease

@mokagio mokagio Jun 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The call on develop is different

WordPress-Android/.circleci/config.yml

Line 58 in a863317

command: ./gradlew testWordpressVanillaRelease --stacktrace --no-daemon

Why did you decide to drop the --stacktrace flag in all the steps and the --no-daemon here?

@oguzkocer oguzkocer Jun 24, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@AliSoftware AliSoftware Jun 24, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

According to a quick git pickaxe, this --no-daemon was:

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.

@mokagio mokagio Jun 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the extra context @AliSoftware @oguzkocer 🙌

@hypest hypest Jun 25, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

- label: "Test WordPressProcessors"
<<: *docker-container
command: |
cp gradle.properties-example gradle.properties
./gradlew :libs:WordPressProcessors:test
- label: "Test ImageEditor"
<<: *docker-container
command: |
cp gradle.properties-example gradle.properties
./gradlew :libs:image-editor:ImageEditor:test