feat(ci): run unit test jobs on arm64 machines additionally #12864

galargh · 2025-02-03T17:26:49Z

Related Issues

Resolves #12860

Proposed Changes

Start running unit-* jobs on arm64 machines in addition to x86_64 ones.

Additional Info

In order to accommodate this change, I moved the test discovery code to a dedicated Go CLI. The important change in the structure of metadata is that now each test group can be associated with many target runners.

What's more, I split the test group discovery into part 1 which lists all the test groups, and part 2 which returns the metadata (e.g. runner information, etc.) for a given test group name. Thanks to this the outputs of the discovery job can be smaller and we minimise the risk of hitting the maximum output size limit.

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title conforms with contribution conventions
Update CHANGELOG.md or signal that this change does not need it per contribution conventions
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

BigLep · 2025-02-03T22:24:48Z

@galargh : I assume you'll mark it ready for review when it is.

galargh · 2025-02-04T09:28:14Z

@galargh : I assume you'll mark it ready for review when it is.

Yes, it is ready for review now :)

masih · 2025-02-04T11:00:04Z

cmd/ci/main.go

+		return [][]string{{"ubuntu-latest"}}
+	}
+
+	namesToRunners := map[string][][]string{


I am not sure how much I like this TBH. Editing yaml for this kind of data seems much more convenient than string hardcoded into map[string][][]string.

From a design perspective, the code here is entirely CI specific. It should live somewhere inside .github folder?

I do think the long term goal is to desolve this into itests themselves in form of annotations or struct tags on "Test Suit" (see testify).

I do think the long term goal is to desolve this into itests themselves in form of annotations or struct tags on "Test Suit" (see testify).

I'd treat this as a mid-step towards that desired goal. It's going to be much easier to parse and manipulate the data from itests here. And then, the hardcoded info inside this tool will disappear.

Yeah, I'd love to see more of this metadata live closer to the tests themselves with annotations, but I understand that is a bigger step. This to me has better readability than jq commands in yaml files we have now. I don't see this making things worse and is actually improving what we have now, so 👍 from me.

The idea of moving this into the .github directory seems good/reasonable.

@galargh : any thoughts on moving this to .github?

BigLep

I took some time coming up to speed on how lotus tests are working within CI. Apologies if I'm raising points that were covered in previous PRs. My comments are partly coming from trying to get my head around the data model and structuring how the tests. Feel free to disregard if my comments are off or don't make sense.

cmd/ci/main.go

.github/workflows/test.yml

BigLep · 2025-02-07T20:19:10Z

.github/workflows/test.yml

+          SKIP_CONFORMANCE: ${{ fromJson(steps.group.outputs.metadata).skip_conformance && '1' || '0' }}
+          TEST_RUSTPROOFS_LOGS: ${{ fromJson(steps.group.outputs.metadata).test_rustproofs_logs && '1' || '0' }}
+          FORMAT: ${{ fromJson(steps.group.outputs.metadata).format || 'standard-verbose' }}
+          PACKAGES: ${{ join(fromJson(steps.group.outputs.metadata).packages, ' ') }}


Why do we set an environment variable when it is also referenced below as a param to gotestsum? Basically I'm wondering if we should inline this to the gotestsum call.

It doesn't matter much here because we literally execute a script to get these values, but it's a good security practice for some types of workflows so I tend to stick with it in general.

Got it - makes sense. I'm good to practice the right behavior here.

Should we adjust this line below to be consistent?

-- ${{ fromJson(steps.group.outputs.metadata).go_test_flags || '' }}

.github/workflows/test.yml

cmd/ci/main.go

BigLep

This is seeming good to me, and certainly not making anything worse. My remaining comments are small nits.

I resolved all previous conversations that I believe are resolved now.

Before merging, it would be good to get input on #12864 (comment)

Also, is there an easy way to sanity check that we aren't reducing our coverage with this change (i.e., we didn't accidentally miss a test group execution context)?

BigLep · 2025-02-10T19:02:25Z

cmd/ci/main.go

 }

+type Runner []string


Maybe add the explanatory comment for why Runner is []string rather than just string?

BigLep · 2025-02-10T19:11:34Z

cmd/ci/main.go

 		},
 	}

-	if strings.HasPrefix(name, "itest-") {
-		return []string{strings.Join([]string{".", "itests", strings.Join([]string{strings.TrimPrefix(name, "itest-"), "test.go"}, "_")}, string(os.PathSeparator))}
+	if strings.HasPrefix(testGroupName, "itest-") {


(nit) but maybe move this if to the top of the function to avoid the interleaving of concerns that we have now

nonItest map declaration
itest handling
nonItest handling

BigLep · 2025-02-10T19:13:48Z

cmd/ci/main.go

+		return []Runner{linux_x64}
+	}
+
+	testGroupNamesToRunners := map[string][]Runner{


nit: testGroupNameToRunner (since the map key is a single test group name?

.github/workflows/test.yml

cmd/ci/main.go

BigLep · 2025-02-10T19:21:51Z

.github/workflows/test.yml

+    runs-on: ${{ matrix.runner }}
+    strategy:
+      matrix:
+        runner: [ubuntu-latest, ubuntu-24.04-arm]


Got it. So this job needs to run on x86_64 and arm, which is accomplished by [ubuntu-latest, ubuntu-24.04-arm]. Sounds good.

Suggested change

runner: [ubuntu-latest, ubuntu-24.04-arm]

# We need to cache for each architecture we support: x86_64 and arm

runner: [ubuntu-latest, ubuntu-24.04-arm]

BigLep · 2025-02-10T19:22:38Z

.github/workflows/test.yml

+          SKIP_CONFORMANCE: ${{ fromJson(steps.group.outputs.metadata).skip_conformance && '1' || '0' }}
+          TEST_RUSTPROOFS_LOGS: ${{ fromJson(steps.group.outputs.metadata).test_rustproofs_logs && '1' || '0' }}
+          FORMAT: ${{ fromJson(steps.group.outputs.metadata).format || 'standard-verbose' }}
+          PACKAGES: ${{ join(fromJson(steps.group.outputs.metadata).packages, ' ') }}


Got it - makes sense. I'm good to practice the right behavior here.

Should we adjust this line below to be consistent?

-- ${{ fromJson(steps.group.outputs.metadata).go_test_flags || '' }}

galargh added 5 commits January 31, 2025 11:52

chore: move test metadata retrieval to a new Go CLI tool

3b77455

ci: install go before retrieving test group metadata

a160e23

ci: run unit tests on arm64 machines

5b65277

ci: fix the packages metadata

ee46bb0

ci: cache deps per runner os and arch

e2290ef

galargh changed the title ~~GitHub actions arm64~~ Run Unit test jobs on arm64 machines additionally Feb 3, 2025

galargh changed the title ~~Run Unit test jobs on arm64 machines additionally~~ feat(ci): run unit test jobs on arm64 machines additionally Feb 3, 2025

BigLep assigned galargh Feb 3, 2025

rvagg mentioned this pull request Feb 4, 2025

Add CI runners for linux and mac arm64 #12860

Open

galargh marked this pull request as ready for review February 4, 2025 09:27

galargh requested review from rvagg and BigLep February 4, 2025 09:27

galargh added the skip/changelog This change does not require CHANGELOG.md update label Feb 4, 2025

masih reviewed Feb 4, 2025

View reviewed changes

BigLep reviewed Feb 7, 2025

View reviewed changes

galargh added 9 commits February 8, 2025 20:06

chore: rename name to testGroupName where applicable

d9bb59c

chore: add createPackagePath helper

35718de

chore: turn runners into an enumerated type

24ca94e

chore: sort testGroupNamesToRunners

debe0ed

chore: rename find/getIntegrationTestGroups

4cb838d

chore: use for-loop to get unit and other test groups

ba81f64

chore: reorganize the ci cli

0df6d19

chore: rename list test groups

70e7618

chore: restore the runner selection

7441e78

BigLep approved these changes Feb 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ci): run unit test jobs on arm64 machines additionally #12864

feat(ci): run unit test jobs on arm64 machines additionally #12864

galargh commented Feb 3, 2025 •

edited

Loading

BigLep commented Feb 3, 2025

galargh commented Feb 4, 2025

masih Feb 4, 2025

galargh Feb 4, 2025

BigLep Feb 7, 2025

BigLep Feb 10, 2025

BigLep left a comment

BigLep Feb 7, 2025

galargh Feb 8, 2025

BigLep Feb 10, 2025

BigLep left a comment •

edited

Loading

BigLep Feb 10, 2025

BigLep Feb 10, 2025

BigLep Feb 10, 2025

BigLep Feb 10, 2025

BigLep Feb 10, 2025

	runner: [ubuntu-latest, ubuntu-24.04-arm]
	# We need to cache for each architecture we support: x86_64 and arm
	runner: [ubuntu-latest, ubuntu-24.04-arm]

feat(ci): run unit test jobs on arm64 machines additionally #12864

Are you sure you want to change the base?

feat(ci): run unit test jobs on arm64 machines additionally #12864

Conversation

galargh commented Feb 3, 2025 • edited Loading

Related Issues

Proposed Changes

Additional Info

Checklist

BigLep commented Feb 3, 2025

galargh commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigLep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BigLep left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galargh commented Feb 3, 2025 •

edited

Loading

BigLep left a comment •

edited

Loading