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.

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

test: try running tests on github action #341

Merged
merged 9 commits into from
Jan 11, 2023

Conversation

kyungeunni
Copy link
Contributor

@kyungeunni kyungeunni commented Jan 4, 2023

Summary

Use github actions for Test step to simplify the process and reduce the maintenance burden

Implementation details

We have had many issues with the testing environment in the past year, often caused by npm changing its behaviour or when using newer node versions (some contexts can be found here )
Having the permission issue #338 recently, and the possible fix #339 causing another issue affecting test results, I wanted to explore the github actions for running tests. I've tried to run the tests on macos(this let us bypass the setup for Xvfb) and It seems quite promising. Here's the impact I've found when opting in for github actions:

Pros:

  • Do not need to build the docker image and maintain it. The docker image is only used by the test step and when we bump the release step, it doesn't run the test step anyways so it won't affect anything else.
  • It can cache the npm modules until package-lock.json changes) so it can reduce the time for checks significantly
  • We can easily reproduce the same behavior as it does on my local machine. previously tests didn't fail at all on CI but failed on my local machine. Running tests locally using the docker image doesn't really work well for this project, as we use a native module(sharp) and the developers use macOS, it throws as the module is not compatible and we might need force-install the native module built for linux.(yet there are some differences even though we do that as docker for Mac behaves differently to linux)

Cons:

  • Won't have CI summary comments created by Jenkins

Next Steps

If we are happy to go forward with github actions, we need to rearrange the Jenkins file and clean-up related scripts, it will be used mainly for the release pipelines(currently release pipeline is triggered when a tag is pushed).

How to validate this change

push to this branch

@apmmachine
Copy link
Contributor

apmmachine commented Jan 4, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-01-11T03:58:27.787+0000

  • Duration: 3 min 58 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /test all : Re-trigger the build for all the stages.

@kyungeunni kyungeunni marked this pull request as ready for review January 4, 2023 05:01
@kyungeunni kyungeunni force-pushed the try-github-action branch 4 times, most recently from bc611bd to 77c141a Compare January 4, 2023 05:25
@kyungeunni
Copy link
Contributor Author

For some reason, gh action is not triggered

@reakaleek
Copy link
Member

reakaleek commented Jan 4, 2023

For some reason, gh action is not triggered

Yes, this is an unfortunate behaviour. When the repository never had a GH actions workflow before.
We usually added a boilerplate workflow and merge it to main to initialize it.

Will create one, and let's try to get it merged.

#345

@kyungeunni
Copy link
Contributor Author

I can see GH workflow working now, thank you, @reakaleek!

What do you suggest we proceed from here? I did the bare minimum of moving running tests to gh workflows, but further insight from the robot teams sounds great as there are certain things need more attention, for instance, do we want to keep all the extra feature we were getting from Jenkins, such as test result summary or alerting etc.

also the tests are failing should be fixed by #346

@reakaleek
Copy link
Member

reakaleek commented Jan 5, 2023

Glad it works now!

Yes, when migrating to GH Actions we want to keep all behaviours the same if possible.

  • Test Summary: For junit test summary report, we already have an established pattern that works with fork PRs in mind.
  • Release Flow: As far as I can see, you only migrated the test part. This will cause the tests to not run when performing a release.
    • We should also aim for migrating this part too. Unfortunately, we actually don't have an established "pattern" for this yet and I'm actually about to design/poc this flow in GH Actions myself.
  • Alerting/Notifications: If there are slack notifications, we definitely want to migrate those too. Any stakeholder might be surprised they are gone otherwise.

If we are okay with missing this functionality temporarily you can go ahead with this one and only apply what is important for you. As I'm currently working mainly on Jenkins to GHActions migrations, I could prioritize this repo and implement/migrate the missing pieces after you merged this.

Note: Looking at my migration spreadsheet I can see the migration for this repo is actually undecided yet (according to the sheet)

@cachedout / @elastic/observablt-ci WDYT?

@kyungeunni
Copy link
Contributor Author

Thanks, @reakaleek!
just one comment on this part:

Release Flow: As far as I can see, you only migrated the test part. This will cause the tests to not run when performing a release.

We are not running tests when releasing even now(see comment here), so this is not a new behavior.

.github/workflows/test.yml Outdated Show resolved Hide resolved
@@ -43,108 +43,6 @@ pipeline {
}
}
}
stage('Install') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing Install, build, and lint stages too, as lint is moved to test.yml and Release stage does npm ci and npm run build within its stage.

kyungeunni and others added 2 commits January 11, 2023 12:57
Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
@kyungeunni
Copy link
Contributor Author

Also I've double-checked that the release CI is unaffected and successfully release the apps.

@kyungeunni kyungeunni merged commit 426065a into elastic:main Jan 11, 2023
@kyungeunni kyungeunni deleted the try-github-action branch January 11, 2023 06:36
@justinkambic
Copy link
Contributor

Skipping Post-FF Testing as this feature is integral to the merge process and if there were problems we'd see/remediate them as part of normal day-to-day workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants