Skip to content

Make environment-dependent Go tests local-friendly#199

Open
jaeyeom wants to merge 1 commit into
gabyx:mainfrom
jaeyeom:fix/local-friendly-go-tests
Open

Make environment-dependent Go tests local-friendly#199
jaeyeom wants to merge 1 commit into
gabyx:mainfrom
jaeyeom:fix/local-friendly-go-tests

Conversation

@jaeyeom

@jaeyeom jaeyeom commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Closes #198

Summary

  • skip the Git hooks documentation test locally when the page cannot be loaded
  • skip the image update test locally when Docker is unavailable, but still fail in CI
  • add a pre-commit hook that runs Go unit tests for staged Go changes

Testing

  • go test -mod=vendor ./hooks
  • GOCACHE=/tmp/gocache CGO_ENABLED=0 go test -mod=vendor ./...

@gabyx gabyx left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for the PR:

The test changes are good and I am fine with them.

However I am a bit unsure if I want a pre-commit hook which runs Go tests cause I am not sure how long they take and also how useful
it is.

Ground truth is always CI, so nothing should be merged anyway to main. At least for me this hook is not that useful and I will anyway disable it if I work in this repo cause, my workflow is change things, break it, fix tests/lints - merge etc... So I don't wont to be bothered by tests with committing.

The other thing (which does not affect merging) is that I work with Nix and the flake.nix inside a DevShell, that means there is never an environment where some tools are missing, which is nice. I work everywhere with Nix shells which are robust, stable, reproducible and make the whole dev cycle a breeze.
You should try it.
I of course understand not everybody wants this awesomeness 😉,
therefore my suggestion:

The pre-commit hook should be option al by an env var. PRECOMMIT_TEST=true or so or removed entirely.
We could have an .env.tmpl file which suggests to set that variable in .env which can be sourced (in the Nix shell) or by other means...

What do you think.

Comment thread .githooks/pre-commit/unittests Outdated
@jaeyeom

jaeyeom commented Apr 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the PR:

The test changes are good and I am fine with them.

However I am a bit unsure if I want a pre-commit hook which runs Go tests cause I am not sure how long they take and also how useful it is.

Ground truth is always CI, so nothing should be merged anyway to main. At least for me this hook is not that useful and I will anyway disable it if I work in this repo cause, my workflow is change things, break it, fix tests/lints - merge etc... So I don't wont to be bothered by tests with committing.

The other thing (which does not affect merging) is that I work with Nix and the flake.nix inside a DevShell, that means there is never an environment where some tools are missing, which is nice. I work everywhere with Nix shells which are robust, stable, reproducible and make the whole dev cycle a breeze. You should try it. I of course understand not everybody wants this awesomeness 😉, therefore my suggestion:

The pre-commit hook should be option al by an env var. PRECOMMIT_TEST=true or so or removed entirely. We could have an .env.tmpl file which suggests to set that variable in .env which can be sourced (in the Nix shell) or by other means...

What do you think.

You're very welcome.

That makes sense.I will delete the unit test pre-commit hook, then. I think anyone who wants this can just add pre-commit hook by themselves rather than PRECOMMIT_TEST=true for added complexity.

The rationale was that unit test should be fast without any dependencies to environments rather than code itself. I will come back with better proposal after a few more iterations.

@jaeyeom jaeyeom force-pushed the fix/local-friendly-go-tests branch from e00bf16 to 41236d7 Compare May 1, 2026 22:56
@jaeyeom

jaeyeom commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Removed the pre-commit unit-tests hook per your feedback. Force-pushed; PR now contains only the test hermeticity fixes (skip-locally / fail-in-CI guards in githooks_test.go and images_test.go).

@jaeyeom

jaeyeom commented May 1, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the PR:
The test changes are good and I am fine with them.
However I am a bit unsure if I want a pre-commit hook which runs Go tests cause I am not sure how long they take and also how useful it is.
Ground truth is always CI, so nothing should be merged anyway to main. At least for me this hook is not that useful and I will anyway disable it if I work in this repo cause, my workflow is change things, break it, fix tests/lints - merge etc... So I don't wont to be bothered by tests with committing.
The other thing (which does not affect merging) is that I work with Nix and the flake.nix inside a DevShell, that means there is never an environment where some tools are missing, which is nice. I work everywhere with Nix shells which are robust, stable, reproducible and make the whole dev cycle a breeze. You should try it. I of course understand not everybody wants this awesomeness 😉, therefore my suggestion:
The pre-commit hook should be option al by an env var. PRECOMMIT_TEST=true or so or removed entirely. We could have an .env.tmpl file which suggests to set that variable in .env which can be sourced (in the Nix shell) or by other means...
What do you think.

You're very welcome.

That makes sense.I will delete the unit test pre-commit hook, then. I think anyone who wants this can just add pre-commit hook by themselves rather than PRECOMMIT_TEST=true for added complexity.

The rationale was that unit test should be fast without any dependencies to environments rather than code itself. I will come back with better proposal after a few more iterations.

I tried with nix. It didn't help me much, unfortunately. Only "format" worked in addition.

What it helped was that I could use "just" and "treefmt" without installing it, which was a small benefit. "go" natively supports downloading the right toolchain versions and tool versions, so nix was unnecessary.

@gabyx

gabyx commented May 3, 2026

Copy link
Copy Markdown
Owner

Some tests are still failing not sure if it can be ignored. For the next week I dont have time to look at it directly

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.

Make environment-dependent Go tests local-friendly

2 participants