Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow local testing #90

Merged
merged 2 commits into from
Jan 28, 2025

Conversation

matthewhughes934
Copy link
Contributor

@matthewhughes934 matthewhughes934 commented Jan 16, 2025

  • Move integration testing to run locally

    And not in Docker. This patch is to prove things work, so I've just
    moved one test across (though most of the changes are in test setup that
    can be re-used across tests). If this gets tractions, more tests could
    be moved across. Reasons for this approach:

    • Simplicity: tests have no external requirements besides git
    • Debugability: running locally you can spin up a debugger (or just
      sleep in tests) and inspect the running environment. This is possible
      with Docker, but more of a hassle
    • Reproducibility: tests are decoupled from the user's environment, so
      failures should be reproducible

    This is achieved via:

    • Using git's worktree's[1] to run things like git commit without
      modifying the working repo
    • Having the tests build a strongbox binary that we can then configure
      git with (built once per run)
    • Adding some test setup with actual keys etc. rather than relying on the
      user having keys define

    For posterity, the commands to setup the keys etc

      go run . -identity-file ./testdata/.strongbox_identity -gen-identity test-key
      grep --only-matching --perl-regexp '(?<=# public key: ).*' ./testdata/.strongbox_identity > ./testdata/.strongbox_recipient
    

    Link: https://git-scm.com/docs/git-worktree [1]

  • Add run of local tests in GitHub

And not in Docker. This patch is to prove things work, so I've just
moved one test across (though most of the changes are in test setup that
can be re-used across tests). If this gets tractions, more tests could
be moved across. Reasons for this approach:

* Simplicity: tests have no external requirements besides `git`
* Debugability: running locally you can spin up a debugger (or just
  sleep in tests) and inspect the running environment. This is possible
  with Docker, but more of a hassle
* Reproducibility: tests are decoupled from the user's environment, so
  failures should be reproducible

This is achieved via:

* Using git's worktree's[1] to run things like `git commit` without
  modifying the working repo
* Having the tests build a strongbox binary that we can then configure
  `git` with (built once per run)
* Adding some test setup with actual keys etc. rather than relying on the
  user having keys define

For posterity, the commands to setup the keys etc

    go run . -identity-file ./testdata/.strongbox_identity -gen-identity test-key
    grep --only-matching --perl-regexp '(?<=# public key: ).*' ./testdata/.strongbox_identity > ./testdata/.strongbox_recipient

Link: https://git-scm.com/docs/git-worktree [1]
@george-angel
Copy link
Member

This is really awesome - love it. If you are happy to move all tests, will merge 👍

@asiyani
Copy link
Contributor

asiyani commented Jan 17, 2025

yes, this is good. I think we should also use TestMain to set defaults, so that we do not override users config while running this or any future tests.
We could also create temp home and set GIT_CONFIG_GLOBAL and STRONGBOX_HOME to that in TestMain?

@matthewhughes934
Copy link
Contributor Author

yes, this is good. I think we should also use TestMain to set defaults, so that we do not override users config while running this or any future tests.

Could you elaborate on this? For clarity: At the moment all the config setup is handled by the configureGit function, which touches the user's local (i.e. in this repo) config just to set extensions.worktreeConfig, then all other configuration updates are limited to the worktree created by the test (i.e. all in a temporary directory).

We could also create temp home and set GIT_CONFIG_GLOBAL and STRONGBOX_HOME to that in TestMain?

Is the a reason for preferring this over the current approach? Currently the files used by STRONGBOX are static and just stored under testdata

@asiyani
Copy link
Contributor

asiyani commented Jan 21, 2025

current approach is individual test will do its own initialisation and config. where as I was suggesting to share some of it between tests using temp home and TestMain.
I am not opposing current approach so if you guys think this is better then its fine by me.

@george-angel
Copy link
Member

Matt can correct me - but I think this is the case because PR is a POC and we'd move this logic to main in full PR.

@matthewhughes934
Copy link
Contributor Author

current approach is individual test will do its own initialisation and config. where as I was suggesting to share some of it between tests using temp home and TestMain. I am not opposing current approach so if you guys think this is better then its fine by me.

👍 I moved the config to be a file under testdata (saves having to configure worktree stuff locally) 3087ce9

@matthewhughes934
Copy link
Contributor Author

This is really awesome - love it. If you are happy to move all tests, will merge

do you want all tests in one PR, or happy to merge this as a starting point and then I can move some more tests on top of this?

@george-angel
Copy link
Member

I would be happy to merge this and have another PR to move the other tests 👍

@matthewhughes934 matthewhughes934 marked this pull request as ready for review January 23, 2025 08:23
Copy link
Member

@george-angel george-angel left a comment

Choose a reason for hiding this comment

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

👏

@george-angel george-angel merged commit 186f91f into uw-labs:master Jan 28, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants