Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Dec 23, 2025

<relative-time> can render absolute dates when passed threshold="P0Y" and prefix="", so remove the previously used <absolute-date> element in its favor.

Devtest before:

Screenshot 2025-12-23 at 20 22 44

Devtest after:

Screenshot 2025-12-23 at 20 22 49

Repo activity (rendering unchanged):

image

https://github.com/gkampitakis/go-snaps is added as test-only dependency because I think snapshot testing is much superiour than manually editing long strings in tests, and you get nice diffs as well in case of mismatch.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/dependencies modifies/frontend labels Dec 23, 2025

actual := du.AbsoluteShort(refTime)
assert.EqualValues(t, `<absolute-date weekday="" year="numeric" month="short" day="numeric" date="2018-01-01T00:00:00Z">2018-01-01</absolute-date>`, actual)
snaps.MatchInlineSnapshot(t, actual, snaps.Inline("<relative-time weekday=\"\" year=\"numeric\" threshold=\"P0Y\" month=\"short\" prefix=\"\" datetime=\"2018-01-01T00:00:00Z\">2018-01-01</relative-time>"))
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 23, 2025

Choose a reason for hiding this comment

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

I don't think it needs snaps.MatchInlineSnapshot

image image

Copy link
Member Author

@silverwind silverwind Dec 23, 2025

Choose a reason for hiding this comment

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

It's more convenient than to manually update strings (and make errors while doing so). We should use it in more places.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more convenient than to manually update strings (and make errors while doing so). We should use it in more places.

No, we don't need it.

"Auto update strings" or "copying unclear data" only makes people lazy and design bad and messy test cases like this and this.

You just don't know what hell they are doing. No one will be able to maintain such code.

Every test cases should be written carefully and be maintainable

image

var failFastBytes = []byte{
0x69, 0x6d, 0x70, 0x6f, 0x72, 0x74, 0x20, 0x6f, 0x72, 0x67, 0x2e, 0x61, 0x70, 0x61, 0x63, 0x68, 0x65, 0x2e, 0x74, 0x6f,
0x6f, 0x6c, 0x73, 0x2e, 0x61, 0x6e, 0x74, 0x2e, 0x74, 0x61, 0x73, 0x6b, 0x64, 0x65, 0x66, 0x73, 0x2e, 0x63, 0x6f, 0x6e,
0x64, 0x69, 0x74, 0x69, 0x6f, 0x6e, 0x2e, 0x4f, 0x73, 0x0a, 0x69, 0x6d, 0x70, 0x6f, 0x72, 0x74, 0x20, 0x6f, 0x72, 0x67,
0x2e, 0x73, 0x70, 0x72, 0x69, 0x6e, 0x67, 0x66, 0x72, 0x61, 0x6d, 0x65, 0x77, 0x6f, 0x72, 0x6b, 0x2e, 0x62, 0x6f, 0x6f,
0x74, 0x2e, 0x67, 0x72, 0x61, 0x64, 0x6c, 0x65, 0x2e, 0x74, 0x61, 0x73, 0x6b, 0x73, 0x2e, 0x72, 0x75, 0x6e, 0x2e, 0x42,
0x6f, 0x6f, 0x74, 0x52, 0x75, 0x6e, 0x0a, 0x0a, 0x70, 0x6c, 0x75, 0x67, 0x69, 0x6e, 0x73, 0x20, 0x7b, 0x0a, 0x20, 0x20,
0x20, 0x20, 0x69, 0x64, 0x28, 0x22, 0x6f, 0x72, 0x67, 0x2e, 0x73, 0x70, 0x72, 0x69, 0x6e, 0x67, 0x66, 0x72, 0x61, 0x6d,
0x65, 0x77, 0x6f, 0x72, 0x6b, 0x2e, 0x62, 0x6f, 0x6f, 0x74, 0x22, 0x29, 0x0a, 0x7d, 0x0a, 0x0a, 0x64, 0x65, 0x70, 0x65,

Copy link
Member Author

Choose a reason for hiding this comment

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

Such huge data assertions are why I want snapshot tests. go test does not a format that can be worked with resonably and just reviewing the snapshot diff is enough to assert whether a result is expected or not.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

I have strong objection of using "go-snaps" to update test data automatically.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 23, 2025
@silverwind
Copy link
Member Author

silverwind commented Dec 23, 2025

Why object snapshot testing? It's clearly superior than tediously manually writing expected strings. I've given up on a number of PRs because it's too much work to to manually write out expected HTML. Take a look at repos like https://github.com/microsoft/typescript-go, 90% of their tests are snapshot-based. It's a huge DX improvement having the ability to "auto update" tests.

@wxiaoguang
Copy link
Contributor

Why object snapshot testing? It's clearly superior than tediously manually writing expected strings. I've given up on a number of PRs because it's too much work to to manually write out expected HTML. Take a look at repos like https://github.com/microsoft/typescript-go, 90% of their tests are snapshot-based. It's a huge DX improvement having the ability to "auto update" tests.

Because many Gitea maintainers are too lazy to write correct tests, not that experienced as typescript-go developers.

But I won't insist, feel free to dismiss my change request if you believe we need it.

@silverwind
Copy link
Member Author

silverwind commented Dec 23, 2025

I think both regular and snapshot tests have their uses. Let's say I want to add another attribute, I need to manually update 6 assertions vs. running this and be done with it:

UPDATE_SNAPS=true go test -count=1 -run TestDateTime ./modules/templates
UPDATE_SNAPS=true go test -count=1 -run TestTimeSince ./modules/templates

BTW there is nothing preventing you from editing the snapshot assertion string, so the string can be updated either manually (if you prefer), or automatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/dependencies modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants