un-skip history test and fix golden mismatches

[imjasonh]

Signed-off-by: Jason Hall jason@chainguard.dev

As discovered in #3779 (comment), these tests were skipped because CI doesn't run in UTC. The failures don't seem to require UTC, only updating the golden file's column spacing to match current reality.

- What I did

Un-skipped tests in history_test.go

- How I did it

Removed skip from history_test.go, added t.Run wrappers so that failed assertions didn't stop other tests from running. Updated golden files to match test expectations.

- How to verify it

make dev && make test-unit

- Description for the changelog

Image history tests are no longer skipped on CI.

- A picture of a cute animal (not mandatory but encouraged)

cc @thaJeztah

[thaJeztah]

Tried this locally, and looks like we may have the same issue as in the other test; the output is formatted for the local timezone, which may not be UTC, in which case the .golden files won't match;

=== RUN   TestNewHistoryCommandSuccess/non-human
    history_test.go:102: assertion failed:
        --- expected
        +++ actual
        @@ -1,3 +1,3 @@
        -IMAGE     CREATED AT             CREATED BY   SIZE      COMMENT
        -abcdef    2017-01-01T12:00:03Z   rose         0         new history item!
        +IMAGE     CREATED AT                  CREATED BY   SIZE      COMMENT
        +abcdef    2017-01-01T13:00:03+01:00   rose         0         new history item!

Not sure what the best solution is for this one though (don't think the .golden files allow for placeholders and/or fuzzy matching 🤔

[imjasonh]

I'm running these tests in the dev env on macOS (make dev), which seems to set the timezone to UTC. Can we have tests run in UTC so they pass consistently?

(I'm also fine with just continuing to skip this test under non-UTC, but we'd still need to fix it so it passes under UTC)

[thaJeztah]

Ah, yes, correct; running these tests inside the dev environment will run them inside a container (which default to use UTC). Running them directly on the host may not be the "common" way to run them, but (e.g.) to run a "one-off" test from within an IDE may be run that way.

I would suggest to either add back the skip for the whole test, or (now that we have subtests) to add a skip for that specific test (although I'd be fine with just adding back the t.Skip if we don't want to complicate things too much).

[imjasonh]

So it turned out that even inside make dev these were being skipped, even though the dev env's timezone was UTC. 🤔

I changed the semantics of notUTCTimezone to check for an offset != 0, which means these run now in the dev env.

https://go.dev/play/p/9o5SSnHUONR

Regardless, whether we skip the test case or not, I think the bigger issue is that it gets skipped in CI, so there's no safeguard against bugs creeping in for non-UTC users (statistically, most of them 🌎). So I'd like to see if we can make this unskipped in CI somehow at least.

edit: https://go.dev/play/p/cJaP36X544S shows why now != now.UTC() may lead to false positives. The extra precision in time.Time gets truncated when UTC-ifying.

[codecov-commenter]

Codecov Report

Merging #3781 (f10ecfd) into master (edd51b2) will increase coverage by 0.04%.
The diff coverage is n/a.

❗ Current head f10ecfd differs from pull request most recent head f5e224e. Consider uploading reports for the commit f5e224e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3781      +/-   ##
==========================================
+ Coverage   59.28%   59.32%   +0.04%     
==========================================
  Files         288      288              
  Lines       24632    24632              
==========================================
+ Hits        14602    14613      +11     
+ Misses       9160     9148      -12     
- Partials      870      871       +1     

[thaJeztah]

Looks like the second commit is missing a DCO sign-off; when you're updating, could you squash the commits, and fix the sign-off to make the DCO check happy?

Thanks again! Sorry for the delay (had a lot of catching up to do on notifications 😅)

[imjasonh]

Friendly ping. Anything else you'd like to see?

[imjasonh]

I'm not sure why DCO isn't reporting. I'll try closing/reopening and/or re-pushing the change to see if that wakes it up.

[imjasonh]

Another friendly ping. I'm not in any rush to get this done, I just keep noticing it's still open. Anything else blocking?

[imjasonh]

Another friendly ping. I'm not in any rush to get this done, I just keep noticing it's still open. Anything else blocking?

Another one. I'm also fine just dropping this if there isn't interest in testing this. If that's the case though, I can also just delete the test.

[thaJeztah]

LGTM

[thaJeztah]

let me close/reopen to have a fresh run of CI 🙈

[thaJeztah]

Whoops; looks like I never pressed the "merge" button on this one.

I was just working on a similar test, and it looks like there's an workaround that just never occurred to me; adding a t.SetEnv("TZ", "UTC") to the test will make the test use UTC. I'll do a quick follow-up after this one's merged 👍