Skip to content

add --merge-json cli flag to support bktec and pants#55

Merged
nprizal merged 4 commits intobuildkite:mainfrom
altana-ai:feat/merge-results
Jun 9, 2025
Merged

add --merge-json cli flag to support bktec and pants#55
nprizal merged 4 commits intobuildkite:mainfrom
altana-ai:feat/merge-results

Conversation

@jasonwbarnett
Copy link
Contributor

This feature is related to adding pants support to test-engine-client (i.e. buildkite/test-engine-client#323)

@jasonwbarnett jasonwbarnett force-pushed the feat/merge-results branch 2 times, most recently from 57d604f to 9fe2fe2 Compare May 31, 2025 12:35
@jasonwbarnett jasonwbarnett changed the title add --merge-json cli flag add --merge-json cli flag to support bktec and pants Jun 2, 2025
Copy link
Contributor

@nprizal nprizal left a comment

Choose a reason for hiding this comment

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

Hi @jasonwbarnett. Thank's for opening this PR. I tested this locally and the file merging is working 🎉. However, I noticed that the .lock file is not being deleted after the process, is this normal? Also do we need create the lock when --merge-json is disabled?

@jasonwbarnett
Copy link
Contributor Author

jasonwbarnett commented Jun 9, 2025

@nprizal Thanks for testing this and for the feedback!

You're absolutely right — I've updated the code so that we only acquire a lock when --merge-json is used.

As for the .lock file being left behind — that's expected behavior when using the filelock package. It doesn’t delete the lock file after releasing it, by design. The presence of the file isn’t what enforces the lock; the lock is handled either via OS-level mechanisms or by writing a PID into the file. Deleting the file can actually introduce race conditions, so it's safest to leave it.

I also made a second change: we're no longer silently rescuing JSON parse errors. If there's malformed JSON in the file, it'll now raise an exception as it should.

Let me know if anything else comes up!

@nprizal
Copy link
Contributor

nprizal commented Jun 9, 2025

@jasonwbarnett Thanks for explaining how the Filelock package works. The changes look good to me. However, there are some lint issues and failed tests on the CI. Could you please have a look?

@jasonwbarnett
Copy link
Contributor Author

🎉 🎉 🎉 🎉

@jasonwbarnett
Copy link
Contributor Author

jasonwbarnett commented Jun 9, 2025

@nprizal Once this PR is merged and a new version is published to pypi.org I can wrap up buildkite/test-engine-client#323 since it requires the newly published package for those tests to pass.

Specific test which is failing due to not having this published: https://buildkite.com/buildkite/test-engine-client/builds/1511/steps/canvas?jid=019756b8-20cd-4086-a4a8-8cff717330ac#019756b8-20cd-4086-a4a8-8cff717330ac/266-289

@nprizal
Copy link
Contributor

nprizal commented Jun 9, 2025

@jasonwbarnett Yes, I've checked your other PR. I'll let you know when this change is published to pypi. Thanks again for your contribution 😃

@nprizal nprizal merged commit 3aa0543 into buildkite:main Jun 9, 2025
11 checks passed
@jasonwbarnett jasonwbarnett deleted the feat/merge-results branch June 9, 2025 23:11
@nprizal nprizal mentioned this pull request Jun 9, 2025
@nprizal
Copy link
Contributor

nprizal commented Jun 9, 2025

@jasonwbarnett The new version has been published to pypi 🚢🐍 https://pypi.org/project/buildkite-test-collector/1.0.4/

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.

2 participants