Skip to content

Conversation

typotter
Copy link
Collaborator

@typotter typotter commented Aug 20, 2024

🎟️ Fixes FF-3090 towards FF-3085

πŸ‘―β€β™‚οΈ Related PRs

Motivation

Changes are often made to the sdk-test-data repository to capture new behaviours, bugs and edge cases. When these changes are pushed to main, the SDKs are cloned locally (locally to the github action running) and their respective tests are run. These tests are set up and run by copies of the SDK test workflows - see sdk-test-data workflow. There are a number of limitations to this setup:

  • Test steps are copied from the SDK’s respective workflows; changes to the SDK test workflows need to be replicated in sdk-test-data and this is not obvious to devs
  • The SDK tests are not able to run against in-flight changes to sdk-test-data
  • When new test-data is committed that breaks an SDK, that breakage is not surfaced in the SDK’s repository until some action triggers its testing workflow (on demand, on pull-request, etc.).

Description of Changes

This change
πŸš€ - Each SDK's testing workflow is enhanced into a reusable workflow, exposing parameters for SDK branch and the sdk-test-data branch to use in testing.

  • The test workflow runs using the main branch of sdk-test-data on all main pushes and Pull Requests using the PR's branch

External to this Change
sdk-test-data get two testing workflows.

  1. ♻️ "Local Testing"- For all pull request changes, the "Local Testing" workflow calls the reusable SDK workflows, test results are recorded only in the sdk-test-data action. This is run using the main SDK branch and the "current" branch of workflow, i.e. the pull request branch. (SDK repo does not see/is not notified of test failures during PR lifecycle, only on push to main)
  2. ♻️ πŸ§‘β€πŸ’» "Remote Testing" - On all pushes to the main branch of sdk-test-data, the SDK testing workflows are triggered to run within their respective repositories, alerting all subscribers of failed runs (repo is red/green).

@@ -5,10 +5,25 @@ on:
paths:
- '**/*'

workflow_dispatch:
Copy link
Collaborator

Choose a reason for hiding this comment

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

πŸ™Œ

@@ -28,4 +50,4 @@ jobs:
distribution: 'adopt'

- name: Run tests
run: make test-data && ./gradlew check --no-daemon --stacktrace
run: make test-data branchName=${{ env.TEST_DATA_BRANCH_NAME }} && ./gradlew check --no-daemon --stacktrace
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to adjust our makefile to support this? Would branchName get overriden with main when it's set in the makefile?

## test-data
testDataDir := src/test/resources/shared
tempDir := ${testDataDir}/temp
gitDataDir := ${tempDir}/sdk-test-data
branchName := main
githubRepoLink := https://github.com/Eppo-exp/sdk-test-data.git
.PHONY: test-data
test-data:
	rm -rf $(testDataDir)
	mkdir -p ${tempDir}
	git clone -b ${branchName} --depth 1 --single-branch ${githubRepoLink} ${gitDataDir}
	cp -r ${gitDataDir}/ufc ${testDataDir}
	rm ${testDataDir}/ufc/bandit-tests/*.dynamic-typing.json
	rm -rf ${tempDir}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok I've learned more about makefiles and passing it in will indeed override πŸ‘Œ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I, too, learned about this nifty feature of Makefiles when I set about this work and really appreciate how easy using Makefiles across the SDKs has made this work!

@typotter typotter merged commit 3f45645 into main Aug 30, 2024
33 checks passed
@typotter typotter deleted the tp/workflows/remote branch August 30, 2024 07:17
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.

3 participants