Skip to content

Conversation

@luke-hill
Copy link
Contributor

πŸ€” What's changed?

⚑️ What's your motivation?

🏷️ What kind of change is this?

  • πŸ“– Documentation (improvements without changing code)
  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • πŸ› Bug fix (non-breaking change which fixes a defect)
  • ⚑ New feature (non-breaking change which adds new behaviour)
  • πŸ’₯ Breaking change (incompatible changes to the API)

♻️ Anything particular you want feedback on?

πŸ“‹ Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@mpkorstanje mpkorstanje self-requested a review October 7, 2025 18:04
GITHUB_SHA=99684bcacf01d95875834d87903dcb072306c9ad
GITHUB_HEAD_REF=main
GITHUB_EVENT_NAME=push
GITHUB_REF=???
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing from this test case because GITHUB_REF is only relevant when a CI job is triggered on a tag.

We're missing a test case for when a build is triggered by a tag.

CI_COMMIT_SHA=123456789
CI_COMMIT_BRANCH=main
CI_JOB_ID=713869896
CI_COMMIT_TAG=???
Copy link
Contributor

@mpkorstanje mpkorstanje Oct 7, 2025

Choose a reason for hiding this comment

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

Missing from this test case because CI_COMMIT_TAG is only relevant when a CI job is triggered on a tag.

We're also missing a test case for when a build is triggered by a tag.

GO_SCM_MY_MATERIAL_PR_BRANCH=aslakhellesoy:bug-fix-42
GO_SCM_MY_MATERIAL_PR_URL=https://github.com/owner/repo/pull/1669
GO_SCM_*_PR_BRANCH=???
GO_SCM_*_PR_URL=???
Copy link
Contributor

Choose a reason for hiding this comment

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

The * matches the branch name. I.e: GO_SCM_MY_MATERIAL_PR_BRANCH So this is a limitation of the script.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Nice find.

It looks like update_testdata.rb got over taken by the complexity of different CI systems. The script effectively checks which variables are used in the bash-like substitutions in CiEnviornments.json. But this assumes all variables are always used or present, which is not the case.

I'll add a few test cases.

@mpkorstanje mpkorstanje changed the title Bugfix/update script Refactor update testdata script Oct 7, 2025
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Made some changes:

  1. Added test cases for Gitlab and Github to cover the situation where a tag is pushed.
  2. Moved the script to testdata to keep concerns contained.
  3. Moved the test data to testdata/src to keep it out of the concerns root.
  4. Updated the integration tests to look for tests in testdata/src

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