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

Add custom CI override functionality #116

Closed
wants to merge 1 commit into from

Conversation

orloffv
Copy link

@orloffv orloffv commented Jan 9, 2025

Add functionality to override isCI variable for custom CI environments.

Add functionality to override `isCI` variable for custom CI environments.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/gkampitakis/go-snaps?shareId=XXXX-XXXX-XXXX-XXXX).
@gkampitakis
Copy link
Owner

Hey 👋 Thanks a lot for the pr. I believe what you try to achieve is already supported by the library without having to expose a new method just for this.

snaps := snaps.WithConfig(snaps.Update(os. Getenv("CUSTOM_CI") == "enabled"))

snaps.MatchSnapshot(t, "conditionally update")

And you can have your own logic for NOT updating a snapshot, of course in the case of detecting running on a CI still this won't be overriden.

https://github.com/gkampitakis/go-snaps/blob/main/snaps/utils.go#L117-L127

@orloffv
Copy link
Author

orloffv commented Jan 15, 2025

Yes, you're right, but I want to cover this case

@gkampitakis
Copy link
Owner

🤔 interesting point let me think about it.

@orloffv
Copy link
Author

orloffv commented Jan 25, 2025

@gkampitakis what do you think?

@gkampitakis
Copy link
Owner

Hey I am sorry for the delay. Didn't have time to look at it. Just for context I am skeptical adding one more "knob" there but your use case is legit. Wondering could you set on your env CI=true? This would have the same results.

@orloffv
Copy link
Author

orloffv commented Jan 28, 2025

Hi @gkampitakis, thanks for getting back to this and considering the use case! 🙌

Setting CI=true in the environment is indeed a valid workaround for some scenarios, but it doesn’t fully address our needs. The core issue is that in our CI pipeline, we don’t have the flexibility to modify environment variables dynamically or override the CI variable due to pipeline constraints. Our CI system runs in a strict and consistent environment without allowing custom variables like CUSTOM_CI.

The purpose of this PR is to provide a lightweight and explicit way to handle these cases, especially for developers who encounter similar limitations. It also avoids tightly coupling custom behavior to the global CI variable, which might be risky if different tools interpret it differently.

That said, I understand the concern about introducing more “knobs.” Would you be open to further discussing how we could make this functionality less intrusive? For example:
• We could introduce it under an experimental flag.
• Or maybe add detailed documentation to clarify when and why to use it?

Looking forward to hearing your thoughts! 😊

@gkampitakis
Copy link
Owner

gkampitakis commented Jan 28, 2025

@orloffv Thanks for your patience!! So I am inclined to say that this would be the correct solution.

- if isCI {
+ if !shouldUpdate(c.update) {
	handleError(t, err)
	return
}
  • Doesn't mess with the globals, it can create weird race conditions
    e.g.
func TestAnother(t *testing.T) {
	t.Parallel()
	snaps.MatchJSON(t, ``)
}

func TestJSON(t *testing.T) {
	t.Parallel()

	snaps.SetCustomCI(true)
....
  • Also gives more flexibility how you can update this value.
  • Doesn't introduce a new config

Technically when you are creating a snapshot you are updating to have a value.

The only difference with your proposed change is that you will need to update your call to snapshots to something like this.

s := snaps.WithConfig(snaps.Update(false))
s.MatchSnapshot(t, "")

Would this be acceptable solution for you?

@orloffv
Copy link
Author

orloffv commented Jan 28, 2025

@gkampitakis That’s better!
Will you create the pull request, or should I?

@gkampitakis
Copy link
Owner

Will work on it asap 👍 Shouldn't take long!

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.

2 participants