@W-11831625@: Ported testing jobs from CircleCI to Github Actions. #843

jfeingold35 · 2022-10-18T19:09:18Z

No description provided.

jfeingold35 · 2022-10-18T19:10:33Z

.github/workflows/run-tests.yml

@@ -0,0 +1,205 @@
+name: run-tests


This runs all the tests, and it has parameters to let us make matrices for node versions. So we could easily create a daily smoke test job that invokes this one the same way the validate_pr one does.

jfeingold35 · 2022-10-18T19:11:22Z

package.json

@@ -74,6 +74,7 @@
 		"@types/tmp": "^0.2.3",
 		"@types/uuid": "^8.3.4",
 		"chai": "^4",
+		"cross-env": "^7.0.3",


Approved 3pp: https://gus.lightning.force.com/lightning/r/ADM_Third_Party_Software__c/a0qEE0000005PCHYA2/view

jfeingold35 · 2022-10-18T19:12:13Z

package.json

@@ -135,6 +136,7 @@
 		"postpack": "rm -f oclif.manifest.json",
 		"lint": "eslint ./src --ext .ts",
 		"test": "./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"",
+		"test-quiet": "cross-env SFGE_LOGGING=false ./gradlew test jacocoTestCoverageVerification && nyc mocha --timeout 10000 --retries 5 \"./test/**/*.test.ts\"",


Using the new variable, we can disable SFGE's internal logging, which is important to avoid overflowing the logs in Github Action.

I can see why this is important. Though when we have failures, the logs would surely help with troubleshooting.
How about writing the output to a file instead of letting it overflow to the console window?

My assumption is that, for a test failure, you'd check the artifact to see what the failures were, and then run those specific tests locally to reproduce.

I suppose my discomfort comes from situations where the issue doesn't repro locally. Can we set a higher threshold for logging rather than completely turning it off?

I'll see how doable that is and get back to you.

jfeingold35 · 2022-10-18T19:32:35Z

.github/workflows/run-tests.yml

+      # Upload the test results and coverage information as artifacts.
+      - name: Upload node test artifacts
+        if: ${{ always() }}
+        uses: actions/upload-artifact@v3


Test results get saved as artifacts, so we can view the specifics of the run by downloading and unzipping those.

jfeingold35 · 2022-10-18T19:32:55Z

.github/workflows/run-tests.yml

+          name: unit-test-dist
+          path: ./dist
+      # Run the unit tests. Use the -quiet variant so SFGE logs don't blow up the console.
+      - name: Run Tests


If the tests fail, these jobs exit with a non-zero code, indicating a failure.

jfeingold35 · 2022-10-18T19:33:25Z

.github/workflows/validate-pr.yml

+name: validate-pr
+on:
+  pull_request:
+    types: [edited, opened, reopened, synchronized]


Runs on basically any PR action.

jfeingold35 · 2022-10-18T19:33:43Z

sfge/build.gradle.kts

-		// Show log4j output during tests
-		showStandardStreams = true
+		// Show log4j output during tests, unless env-var to disable them is set.
+		showStandardStreams = (System.getenv("SFGE_LOGGING") != "false")


This is the part where the new env-var is used.

jfeingold35 · 2022-10-18T19:35:45Z

test/lib/formatter/RuleResultRecombinator.test.ts

 				const results = (await RuleResultRecombinator.recombineAndReformatResults(retireJsVerboseViolations, OUTPUT_FORMAT.HTML, new Set(['retire-js']), true)).results as string;
-				const violationString = results.split("const violations = [")[1].split("];\n")[0];
+				const violationString = results.split("const violations = [")[1].split("];")[0];


This was failing in Windows, on Powershell. I believe the problem was that because Windows uses \r\n instead of \n, the split was failing.

jfeingold35 · 2022-10-18T19:35:59Z

test/lib/services/LocalCatalog.test.ts

@@ -7,7 +7,7 @@ import { ENGINE, LANGUAGE } from '../../../src/Constants';
 import { Rule, RuleGroup } from '../../../src/types';
 import LocalCatalog from '../../../src/lib/services/LocalCatalog';
 import { expect } from 'chai';
-import {Controller} from '../../../lib/Controller';
+import {Controller} from '../../../src/Controller';


This import path was wrong.

jfeingold35 · 2022-10-27T21:21:49Z

.github/workflows/run-tests.yml

@@ -0,0 +1,203 @@
+name: run-tests
+on:
+  workflow_call:


workflow_call lets us reuse this across all of the places where we might want to run tests, which is pretty helpful.

jfeingold35 · 2022-10-27T21:22:39Z

.github/workflows/publish-to-npm.yml

+
+jobs:
+  # Step 1: Verify that the tag we're trying to release is a valid candidate for publishing.
+  verify-candidate-tag:


This is pretty closely copied from validate-candidate-branch.sh, just split into a job and converted to use tags.

jfeingold35 · 2022-10-27T21:24:00Z

.github/workflows/publish-to-npm.yml

+          PACKAGE_VERSION=v`cat package.json | jq '.version' | xargs`
+          [[ ${TAG} == ${PACKAGE_VERSION} ]] || (echo "Tag name must match package.json version, prefixed by lowercase v" && exit 1)
+  # Step 2: Publish the tag as a release candidate.
+  publish-rc:


This job is analogous to an example job provided by the CLI team, with the only significant difference being that we use the latest-rc tag, whereas they use latest.

rmohan20 · 2022-11-02T21:56:11Z

.github/workflows/run-tests.yml

+      - uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: '11' # For now, Java version is hardcoded. It could be changed to a matrix in the future.


Shouldn't this be 8?

I'm pretty sure this should be 11. But we could matrixify it so it uses 8 and 11.

rmohan20 · 2022-11-02T22:11:10Z

.github/workflows/run-tests.yml

+    needs: build-dependencies
+    steps:
+      # Check out the code.
+      - uses: actions/checkout@v2


Reading around, I see there's a actions/checkout@v3 as well. Is a specific reason to use v2 instead of v3?

Noticed a combination of v3 in the release workflow and v2 in the test workflow.

No. I'll switch to v3 throughout.

rmohan20 · 2022-11-02T22:12:39Z

.github/workflows/run-tests.yml

+      - uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: '11' # For now, Java version is hardcoded. It could be changed to a matrix in the future.


Same as above - should this be 8?

rmohan20 · 2022-11-02T22:34:02Z

.github/workflows/run-tests.yml

+          # If the path starts with C:, we need to rip that off (needed for Windows).
+          ADJUSTED_TARBALL_PATH=`[[ $RAW_TARBALL_PATH = C* ]] && echo $RAW_TARBALL_PATH | cut -d':' -f 2 || echo $RAW_TARBALL_PATH`
+          # Pipe in a `y` to simulate agreeing to install an unsigned package. Use a URI of the file's full path.
+          echo y | sfdx plugins:install "file://${ADJUSTED_TARBALL_PATH}/${TARBALL_NAME}"


Can windows fail with the / in the path instead of \?

In general, for this block, how about gating with an "if os == windows" check?

I never saw it fail, so I think it should be fine without.

.github/workflows/publish-to-npm.yml

rmohan20 · 2022-11-02T22:53:18Z

.github/workflows/publish-to-npm.yml

+      - uses: actions/setup-java@v3
+        with:
+          distribution: 'temurin'
+          java-version: '11' # For now, Java version is hardcoded.


Should this be 8?

~~Probably. Good catch.~~
Edit: Actually, no I think it should be 11. Because this is the RC test, not the building for publishing. We want to test with v11, right?
(Alternatively, we could matrixify this and test with both 8 and 11. Thoughts?)

Ah, I see why 11 then. Agree that this doesn't have to be 8. I like the idea of testing with multiple java versions. Especially to keep up with the later versions. Would it be easy to include now while we are here?

Shouldn't be too hard.

rmohan20

So many details covered, this looks great! I had a few minor questions.

jfeingold35 · 2022-11-03T18:59:10Z

github-actions/verify-pr-title/package.json

@@ -13,7 +13,7 @@
 	},
 	"dependencies": {
 		"@actions/core": "^1.2.4",
-		"@actions/github": "^4.0.0"
+		"@actions/github": "^5.0.0"


Incrementation was necessary to bypass this issue: octokit/plugin-rest-endpoint-methods.js#172

jfeingold35 · 2022-11-03T18:59:35Z

github-actions/verify-pr-title/src/index.ts

-		const title = pullRequest.title;
-		const baseBranch = pullRequest.base.ref;
-		if (verifyPRTitleForBugId(title) && verifyPRTitleForBadTitle(title) && verifyPRTitleForBaseBranch(title, baseBranch)) {
+		const title = pullRequest.title as string;


Linting fixes.

jfeingold35 · 2022-11-03T19:00:04Z

github-actions/verify-pr-title/test/verifyPrTitle.test.ts

@@ -31,14 +30,6 @@ describe("Positive Tests", () => {
 	it("Title does not start with invalid tokens d/W or r/W", () => {
 		expect(verifyPRTitleForBadTitle("@W-12345678")).to.equal(true);
 	});
-


Removed unneeded tests.

jfeingold35 commented Oct 18, 2022

View reviewed changes

@W-11831625@: Ported testing jobs from CircleCI to Github Actions.

a03837d

jfeingold35 force-pushed the d/W-11831625-a branch from ce82b39 to a03837d Compare October 25, 2022 19:33

jfeingold35 added 2 commits October 25, 2022 16:54

@W-11831625@: Added daily smoke tests as Github Actions.

8528c7b

@W-11831625@: Added release process as Github action.

8b48910

jfeingold35 commented Oct 27, 2022

View reviewed changes

@W-11831625@: Updated branch name references.

d69e30f

jfeingold35 marked this pull request as ready for review November 2, 2022 19:45

rmohan20 reviewed Nov 2, 2022

View reviewed changes

.github/workflows/publish-to-npm.yml Show resolved Hide resolved

rmohan20 reviewed Nov 2, 2022

View reviewed changes

jfeingold35 added 2 commits November 3, 2022 13:50

@W-11831625@: Fixed typos and errors in Github Actions.

ca4294b

@W-11831625@: Partial response to code review feedback.

0ca8253

jfeingold35 force-pushed the d/W-11831625-a branch from 027e323 to 0ca8253 Compare November 3, 2022 18:52

jfeingold35 commented Nov 3, 2022

View reviewed changes

rmohan20 approved these changes Nov 3, 2022

View reviewed changes

jfeingold35 merged commit 0e952e1 into dev Nov 3, 2022

jfeingold35 mentioned this pull request Nov 7, 2022

CHANGE (CodeAnalyzer): @W-11326833@: We more strongly enforce PR naming conventions. #873

Merged

jfeingold35 deleted the d/W-11831625-a branch February 22, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@W-11831625@: Ported testing jobs from CircleCI to Github Actions. #843

@W-11831625@: Ported testing jobs from CircleCI to Github Actions. #843

jfeingold35 commented Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022

rmohan20 Nov 3, 2022

jfeingold35 Nov 3, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 18, 2022

jfeingold35 Oct 27, 2022

jfeingold35 Oct 27, 2022

jfeingold35 Oct 27, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022

rmohan20 Nov 2, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022

rmohan20 Nov 2, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022

rmohan20 Nov 2, 2022

jfeingold35 Nov 3, 2022 •

edited

Loading

rmohan20 Nov 3, 2022

jfeingold35 Nov 3, 2022

rmohan20 left a comment

jfeingold35 Nov 3, 2022

jfeingold35 Nov 3, 2022

jfeingold35 Nov 3, 2022

@W-11831625@: Ported testing jobs from CircleCI to Github Actions. #843

@W-11831625@: Ported testing jobs from CircleCI to Github Actions. #843

Conversation

jfeingold35 commented Oct 18, 2022

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfeingold35 Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmohan20 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

jfeingold35 Nov 3, 2022 •

edited

Loading