Test current code base as an integration test for PRs and pushes #505

Pike · 2020-11-18T14:45:16Z

Description

Add a build step to create lib and node_modules artifact
Add an integration test to run on PRs

Testing Instructions

Happens as part of this PR.

Additional Notes

I bet the integration test will fail for insufficient permissions. We'll see.

codecov-io · 2020-11-18T14:46:31Z

Codecov Report

❗ No coverage uploaded for pull request base (dev-v4@7a94841). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             dev-v4      #505   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?         6           
  Lines             ?       179           
  Branches          ?        50           
==========================================
  Hits              ?       179           
  Misses            ?         0           
  Partials          ?         0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a94841...62dc44b. Read the comment docs.

Pike · 2020-11-18T16:22:36Z

So, this failed in a good way.

Things I like about this PR: It indicates that just installing the prod dependencies should do the trick for the action (3.7.1 ships dev dependencies, too). It runs an integration test.

I'm tempted to stack dry-run on top to allow for the integration test to actually pass :-) . @JamesIves ?

JamesIves · 2020-11-19T12:03:32Z

I think adding the dry run stuff to this PR will be the only way this will work unfortunately, but I'm a big fan of this. It should really help! I'm thinking we should expand this to run the entire integration test suite and not just a single one.

The release branch gets updated manually by me whenever I merge something into it (I just switch and run yarn install). I've been wanting to automate that process as the action could in theory deploy the dependencies its self in a similar manor to this whenever content is merged into that branch.

Thanks for this, it's really appreciated!

JamesIves · 2020-11-19T12:05:03Z

.github/workflows/build.yml

+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v1


We should run v2 here with persist-credentials: false as it's the latest version; https://github.com/JamesIves/github-pages-deploy-action/blob/dev/.github/workflows/integration.yml#L42-L45

JamesIves · 2020-11-19T12:06:55Z

.github/workflows/build.yml

+        with:
+          FOLDER: integration
+          BASE_BRANCH: ${{ github.sha }}
+          BRANCH: gh-pages-pr


We should consider adding a cleanup step after this in the off chance that it does actually generate these branches (ie if someone with adequate permissions made a PR). This is a potential window for abuse so it's probably best we prevent things from lingering: https://github.com/JamesIves/github-pages-deploy-action/blob/dev/.github/workflows/integration.yml#L31-L35

Pike · 2020-12-07T16:28:15Z

I've spun dry-run into its own PR #526 , with some unit tests.

With that, I wonder if I should move the steps into the integration.yml workflow completely?

JamesIves · 2020-12-08T15:45:52Z

Lemme gather my thoughts on this, I'll take a closer look later today!

JamesIves · 2020-12-09T15:17:39Z

Quick question before I merge: Should this include the dry-run flag? I think keeping this in build/PR makes sense as the intention of this is to smoke test PR's instead of running the scheduled tests.

Pike · 2020-12-09T15:25:24Z

I think this should, yes, otherwise the tests won't pass.

Also, I still need to address your review comments, let me get to that real quick (I'm actually hacking on this right now 😄 )

Pike · 2020-12-09T15:40:51Z

Soooo, --dry-run isn't as dry run as I thought it was, it does fail for failed permissions. I filed #536 on that, should I tackle this as part of this PR? Sounds like doing it here is the best to avoid more chickens and eggs?

JamesIves · 2020-12-09T16:16:48Z

Yeah tackling it as part of this PR makes the most sense to me!

Pike · 2020-12-09T16:21:32Z

Nice, I'm finding actual bugs now. Also bugs I introduced.

Pike · 2020-12-09T16:30:01Z

For reference, generateBranch runs in workspace, but needs to run in temporaryDeploymentDirectory, now that we don't mess with workspace anymore. More refactorings coming up, sorry for that.

Pike · 2020-12-10T16:26:23Z

I'm putting this into draft state for now, I need to do a couple of iterations here, probably.

I'll update the PR here soon, but I'm also changing a lot, so feel free to comment and complain liberally.

The high-level idea is that the mix of worktree and generateBranch and singleCommit turned out to be hard to untangle for me. So I'm switching over to a generateWorktree, and then a rather different logic for singleCommit. Basically, I'm starting off with an orphan branch right away, and then use hg diff to figure out if to actually commit or not.

JamesIves · 2020-12-10T16:48:58Z

Sounds good to me, I'll keep an eye out. I've got a lot of spare time coming up this weekend so I'm hoping to knock out a lot of these remaining tasks for v4.

For pull requests, the github.sha is the sha of the merge to the target branch, not the head of the PR. Special case that.

I also made the branches more generic, as there are now more of them.

Also add tests for dryRun and singleCommit and generateBranch code flows.

…ngleCommit This is a continuation of the no-checkout work, and sadly suggested pretty intensive changes.

Pike · 2020-12-10T21:21:21Z

I think this is in a shape where I'd appreciate feedback. Candid-european is perfectly fine.

A few thoughts I have:

I think the dry-run integration testing turned out rather well. I wish it was more strict, which might be be OK to do with checking the action output. Right now it's an environment, in particular with multiple deploy steps, that's interesting. Is there a reason to not make it an step output? I also entertained the idea to expose the count of commits done as step output, which could be checked for things like singleCommit or not.

I introduced an abstraction of a git command. It's not placed anywhere logical, but just close to where it's used. I like being able to set options one by one, and then have the class generate the command string. I do think I only did this half-way, though. execute could probably take that abstraction|string as argument, and toString() itself. I'm not sure if this pays off in other parts of the code. In generateWorktree, there's a mix between --orphan and -B that might be more confusing to write as a string composition..

I also like the actual testing of createWorktree with actual git content. With that being properly (IMHO) tested by itself, it'd be nice to mock it in git.test.ts, but I haven't found a way to do that.

Also, I'm a bit lost on how to test the error handling in createWorktree.

JamesIves · 2020-12-11T03:33:39Z

I will take a look first thing tomorrow when I wake up.

JamesIves

This looks really good, and I really appreciate you taking the time on this. I have a few comments/questions.

Regarding testing the error handling there's a way you can make the execute function throw if it's given a specific string, I believe it's done here: https://github.com/JamesIves/github-pages-deploy-action/blob/dev/__tests__/git.test.ts#L56-L58

As we can't necessarily test git failures just ensuring that we have coverage on the catch would be significant, that way we know it will bubble up to the calling function and provide the user with an adequate error message.

JamesIves · 2020-12-11T14:52:33Z

__tests__/worktree.test.ts

+  beforeAll(async () => {
+    const silent = true
+    tempdir = fs.mkdtempSync(path.join(os.tmpdir(), 'gh-deploy-'))
+    const origin = path.join(tempdir, 'origin')
+    await execute('git init origin', tempdir, silent)
+    await execute('git config user.email "you@example.com"', origin, silent)
+    await execute('git config user.name "Jane Doe"', origin, silent)
+    await execute('git checkout -b main', origin, silent)
+    fs.writeFileSync(path.join(origin, 'f1'), 'hello world\n')
+    await execute('git add .', origin, silent)
+    await execute('git commit -mc0', origin, silent)
+    fs.writeFileSync(path.join(origin, 'f1'), 'hello world\nonce more\n')
+    await execute('git add .', origin, silent)
+    await execute('git commit -mc1', origin, silent)
+    await execute('git checkout --orphan gh-pages', origin, silent)
+    await execute('git reset --hard', origin, silent)
+    await fs.promises.writeFile(path.join(origin, 'gh1'), 'pages content\n')
+    await execute('git add .', origin, silent)
+    await execute('git commit -mgh0', origin, silent)
+    await fs.promises.writeFile(
+      path.join(origin, 'gh1'),
+      'pages content\ngoes on\n'
+    )
+    await execute('git add .', origin, silent)
+    await execute('git commit -mgh1', origin, silent)


Can you explain to me so I understand what's happening here. Is this setting up the requirements in the env for generateWorkTree to run? It feels weird to manually run a sequence of git commands like this but I understand the need if it's just required setup.

If that is the case we should probably comment this file to indicate such.

Yeah, I'm adding a bunch of comments to the test file now.

JamesIves · 2020-12-11T14:52:56Z

__tests__/worktree.test.ts

+    fs.writeFileSync(path.join(origin, 'f1'), 'hello world\n')
+    await execute('git add .', origin, silent)
+    await execute('git commit -mc0', origin, silent)
+    fs.writeFileSync(path.join(origin, 'f1'), 'hello world\nonce more\n')


Suggested change

fs.writeFileSync(path.join(origin, 'f1'), 'hello world\nonce more\n')

fs.writeFileSync(path.join(origin, 'f1'), 'hello world\once more\n')

This is a very unfortunate typo 😛

See, this happens when you're not a native speaker. Not really a typo, I'll just use \nand planets instead.

haha yeah totally understand

JamesIves · 2020-12-11T15:15:54Z

src/git.ts

+  constructor(branch: string) {
+    this.branch = branch
+  }
+  toString(): string {


Not sure I love naming this toString, but I get the idea of what's happening here. 😬

JamesIves · 2020-12-11T15:20:02Z

src/git.ts

+    // Special case is singleCommit with existing history, when
+    // we're really interested if the diff against the upstream branch
+    // changed.
+    const checkCmd =


This might be better named checkGitStatus or something more descriptive.

JamesIves · 2020-12-11T15:21:23Z

src/git.ts

-    )
-    await execute(`git fetch`, action.workspace, action.silent)
-
-    info(`Created the ${action.branch} branch… 🔧`)


We should keep the logging in place when a branch is created

JamesIves · 2020-12-11T15:22:26Z

src/git.ts

@@ -33,34 +33,79 @@ export async function init(action: ActionInterface): Promise<void | Error> {
  }
 }

-/* Generates the branch if it doesn't exist on the remote. */
-export async function generateBranch(action: ActionInterface): Promise<void> {
+export class GitCheckout {


I think it would be a good idea to throw this class and its methods into a worktree.ts file as an associated worktree.test.ts file already exists. That way each test file tests its adjacent functions/classes.

JamesIves · 2020-12-11T15:28:07Z

.github/workflows/build.yml

+        with:
+          FOLDER: integration
+          BRANCH: ${{ matrix.branch }}
+          SINGLE_COMMIT: ${{ matrix.commit == 'singleCommit' }}


Pike · 2020-12-11T16:15:44Z

I'm going to just add an explicit test file for error handing. The problem (AFAICT) is that you can only mock modules per test file. So I'm having one test file that doesn't mock execute.ts, and one test file that does.

…nch.

Pike · 2020-12-11T16:37:01Z

Thanks for the review, I think I've got them all.

I'm not all that fond of c3cddcb, but I also don't have a better idea. W/out that, coverage complains about the conditional check for changes files. I made that a separate commit so it's easier to mess with.

Pike · 2020-12-11T17:27:46Z

Another idea I just had was to mock

jest.mock('../src/worktree', () => ({
  // eslint-disable-next-line @typescript-eslint/naming-convention
  __esModule: true,
  generateWorktree: jest.fn()
}))

in git.test.ts. That's an opportunity now that generateWorktree is in its own module.That would decouple the tests further, in particular if the commands in generateWorktree changed. Not sure it does much more than that. Well, it cuts down the different call counts to execute from 8, 10, 11, 12 to 4, 7, 8

JamesIves · 2020-12-13T02:44:52Z

Thanks for the review, I think I've got them all.

I'm not all that fond of c3cddcb, but I also don't have a better idea. W/out that, coverage complains about the conditional check for changes files. I made that a separate commit so it's easier to mess with.

There is already a isTest variable, would that be appropriate instead of adding another?

For the most part this is looking great! I'll merge this Sunday afternoon and get a pre-release up for this so we can start some more in-depth testing.

Pike · 2020-12-13T13:12:03Z

There is already a isTest variable, would that be appropriate instead of adding another?

I've been wondering about that a bit. Right now, isTest is basically hasChangedFilesForTest?

I'm not too fond of using the same value for both test flags, but we could use an enum like

enum TestFlag {
  NONE = 0,
  HAS_CHANGED_FILES = 1 << 1,
  HAS_REMOTE_BRANCH = 1 << 2,
}

Than we only need one variable, but can combine different scenarios in one test. It's also explicit when setting up the test?

JamesIves · 2020-12-14T15:23:27Z

I like the use of the enum!

Pike · 2020-12-14T17:21:15Z

I have mixed feelings on whether a required or an optional isTest flag is worse. TS complains about action.isTest & TestFlag.HAS_CHANGED_FILES if it's optional, which makes the code harder to read if we'd work around that. And requiring it makes the tests more verbose.

The .eslintrc.json change is something I found on the net when trying to fix the new exception, and it seems to work w/out exceptions and details now.

JamesIves · 2020-12-14T17:29:31Z

I see what you mean, it is somewhat confusing to have the two. I think for now let's merge this, and in a follow-up figure out if we can deprecate the use case of isTest as it's a bit of a cheeky workaround to begin with anyway. I will take care of that as I wrote it and I think it's just used for getting test coverage, but a better handling of mocking would be more appropriate.

JamesIves · 2020-12-14T17:38:38Z

Now in beta 2: https://github.com/JamesIves/github-pages-deploy-action/releases/tag/4.0.0-beta-02

* Stop checking out workspace (#515) * Stop checking out base branch before deployment, drop option. * Don't check out default branch, as we don't check out base branch, drop option. * Don't stash/unstash as we don't update the workdir, drop preserve option. * Don't init the workspace * Only fetch the remote branch if it exists, only with depth 1. * Rely on previous checkouts to have handled lfs files correctly, drop option. * Update README, action.yml, integration tests * Set up eslint for test files. (#517) * Add DRY_RUN option, passing --dry-run to git push. (#526) See #499 for the proposal. * Simplifies Token Setup (#530) * Token simplification * Access Token / Github Token -> Token * Oops * Typos * Update README.md * Update README.md * Update action.yml Co-authored-by: Axel Hecht <axel@pike.org> * Update README.md Co-authored-by: Axel Hecht <axel@pike.org> * Update README.md Co-authored-by: Axel Hecht <axel@pike.org> * Adjust codeql action to latest recommendations (#540) Also, add the dev and release branches, and drop master. * Add workflow to update build and node_modules on release branches (#541) * Stores username/email in secrets * Removing stale bot integration * Test current code base as an integration test for PRs and pushes (#505) * Add a build step to create lib and node_modules artifact * Run integration test with built dist and current SHA as base For pull requests, the github.sha is the sha of the merge to the target branch, not the head of the PR. Special case that. * Use v2 checkout, and DRY_RUN for the integration test. I also made the branches more generic, as there are now more of them. * Fix #536, don't push at all on dryRun Also add tests for dryRun and singleCommit and generateBranch code flows. * Try to fix dryRun on new remote branches, refactor fetch * Try to fix dryRun, only fetch if origin branch exists * Refactor worktree setup to include branch generation and setup for singleCommit This is a continuation of the no-checkout work, and sadly suggested pretty intensive changes. * Set up git config to fix tests, also make debugging easier * Add matrix for existing and non-existing branch * Add matrix for singleCommit and not * Drop GITHUB_TOKEN, add DRY_RUN to action.yml * When deploying existing branch, add a modifcation and deploy again * Force branch checkout to work in redeployment scenarios * Make singleCommit easier to see in job descriptions * Review comments * Add a test-only property to action to test code paths with remote branch. * Introduce TestFlag enum to signal different test scenarios to unit tests * Fix util.test.ts * Update worktree.ts * Fix a few nits in tests and automation. Don't try to wordcount ls-rem… (#546) * Fix a few nits in tests and automation. Don't try to wordcount ls-remote. Nits in tests are around undoing changes made to the environment, and to not modify the checkout. * Describe suite with empty SHA * Lowercase Inputs (#547) * Lowercases inputs * Adjusts workflow tests and deployment_status * Use multi-line string for clean-exclude patterns. (#553) As this change is subtle, I'm taking the opportunity to change the underscore for the hyphen, which makes it less likely that users of this action will just pass in an old json array. * Hyphenate inputs and outputs, add step output, fix #558 (#559) * Hyphenate inputs and outputs, add step output, fix #558 I've also tried to make the clean docs a bit clearer, and consistent about clean being on my default. Still not totally happy with the intro of the docs there, though. * Add testing of step outputs to build integration tests * Security Docs * Integration tests * Revert "Integration tests" This reverts commit 639ff53. * Native SSH Key Support (#569) * SSH Key Support 🔑 * Update ssh.ts * Update src/ssh.ts Co-authored-by: Axel Hecht <axel@pike.org> * README fixes/etc * Unit Tests & README * ssh key * Update README.md * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update ssh.test.ts * Update integration.yml Co-authored-by: Axel Hecht <axel@pike.org> * Deployment Issues (#583) * Update git.ts * Tests * Update git.ts * Formatting * Update src/git.ts Co-authored-by: Axel Hecht <axel@pike.org> * TestFlag * Logging * Update git.ts Co-authored-by: Axel Hecht <axel@pike.org> * Codespace Support (#584) * Add files via upload * Update README.md * Add files via upload * Update README.md * Update README.md * Update README.md * Update README.md * Update README.md * SSH Issues (#588) * Unsets Persisted Credentials (#587) * Persist * Config Setup/Tests * Assets * Update git.ts * Spacing * Update integration.yml * Update README.md Co-authored-by: Axel Hecht <axel@pike.org>

Pike requested a review from JamesIves as a code owner November 18, 2020 14:45

JamesIves reviewed Nov 19, 2020

View reviewed changes

Pike mentioned this pull request Nov 30, 2020

Stop checking out workspace #515

Merged

JamesIves added the version 4 Issues related to version 4 of this action. label Nov 30, 2020

JamesIves changed the base branch from dev to dev-v4 December 1, 2020 14:26

JamesIves added the testing 🧪 This change is being tested. label Dec 8, 2020

JamesIves added this to the 4.0.0 milestone Dec 8, 2020

Pike marked this pull request as draft December 10, 2020 16:20

Pike added 8 commits December 10, 2020 17:59

Add a build step to create lib and node_modules artifact

9555eaa

Run integration test with built dist and current SHA as base

98981bf

For pull requests, the github.sha is the sha of the merge to the target branch, not the head of the PR. Special case that.

Use v2 checkout, and DRY_RUN for the integration test.

eb69ad0

I also made the branches more generic, as there are now more of them.

Fix #536, don't push at all on dryRun

3e801c2

Also add tests for dryRun and singleCommit and generateBranch code flows.

Try to fix dryRun on new remote branches, refactor fetch

35e3262

Try to fix dryRun, only fetch if origin branch exists

e05fe6c

Refactor worktree setup to include branch generation and setup for si…

af3f111

…ngleCommit This is a continuation of the no-checkout work, and sadly suggested pretty intensive changes.

Set up git config to fix tests, also make debugging easier

13ef90f

Pike added 6 commits December 10, 2020 19:58

Add matrix for existing and non-existing branch

3e8c35c

Add matrix for singleCommit and not

3a5a636

Drop GITHUB_TOKEN, add DRY_RUN to action.yml

5020986

When deploying existing branch, add a modifcation and deploy again

a076bb2

Force branch checkout to work in redeployment scenarios

1005df9

Make singleCommit easier to see in job descriptions

174f40e

Pike marked this pull request as ready for review December 10, 2020 20:59

Pike mentioned this pull request Dec 10, 2020

DRY_RUN fails on missing permissions #536

Closed

JamesIves reviewed Dec 11, 2020

View reviewed changes

Pike added 2 commits December 11, 2020 17:25

Review comments

0d0fbbc

Add a test-only property to action to test code paths with remote bra…

c3cddcb

…nch.

Pike added 2 commits December 14, 2020 17:53

Introduce TestFlag enum to signal different test scenarios to unit tests

0064e3d

Fix util.test.ts

62dc44b

JamesIves merged commit 4e40ddd into JamesIves:dev-v4 Dec 14, 2020

Pike deleted the test-pr branch December 14, 2020 17:58

Pike mentioned this pull request Dec 14, 2020

Better mock for unittests #544

Closed

	fs.writeFileSync(path.join(origin, 'f1'), 'hello world\nonce more\n')
	fs.writeFileSync(path.join(origin, 'f1'), 'hello world\once more\n')

Test current code base as an integration test for PRs and pushes #505

Test current code base as an integration test for PRs and pushes #505

Conversation

Pike commented Nov 18, 2020

codecov-io commented Nov 18, 2020 • edited Loading

Codecov Report

Pike commented Nov 18, 2020

JamesIves commented Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

JamesIves Nov 19, 2020 • edited Loading

Choose a reason for hiding this comment

Pike commented Dec 7, 2020

JamesIves commented Dec 8, 2020

JamesIves commented Dec 9, 2020

Pike commented Dec 9, 2020

Pike commented Dec 9, 2020

JamesIves commented Dec 9, 2020

Pike commented Dec 9, 2020

Pike commented Dec 9, 2020

Pike commented Dec 10, 2020

JamesIves commented Dec 10, 2020

Pike commented Dec 10, 2020

JamesIves commented Dec 11, 2020 • edited Loading

JamesIves 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

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

Pike commented Dec 11, 2020

Pike commented Dec 11, 2020

Pike commented Dec 11, 2020

JamesIves commented Dec 13, 2020 • edited Loading

Pike commented Dec 13, 2020

JamesIves commented Dec 14, 2020

Pike commented Dec 14, 2020

JamesIves commented Dec 14, 2020 • edited Loading

JamesIves commented Dec 14, 2020

codecov-io commented Nov 18, 2020 •

edited

Loading

JamesIves commented Nov 19, 2020 •

edited

Loading

JamesIves Nov 19, 2020 •

edited

Loading

JamesIves commented Dec 11, 2020 •

edited

Loading

JamesIves commented Dec 13, 2020 •

edited

Loading

JamesIves commented Dec 14, 2020 •

edited

Loading