Update TestNormalize to only test Windows platform #6569
Conversation
platforms/platforms_test.go
Outdated
| @@ -367,5 +367,13 @@ func TestParseSelectorInvalid(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestNormalize(t *testing.T) { | |||
| require.Equal(t, DefaultSpec(), Normalize(DefaultSpec())) | |||
| spec := DefaultSpec() | |||
There was a problem hiding this comment.
How about hard-coding the spec here instead of using DefaultSpec? Changing TestNormalize() based on the underlying architecture doesn't make much sense.
There was a problem hiding this comment.
I guess maybe we should step back and understand what this is expected to test; seems from the origin PR this is about making sure we aren't losing the info for Windows platform objects on normalization. One option could be to just add this test for Windows and not other platforms? @thaJeztah ?
There was a problem hiding this comment.
Right, so @tonistiigi mentioned it was broken on ARM #6497 (comment), and I recall trying to make the test work in CI in this repo, but it depends on actual ARM hardware to do so (not GOOS/GOARCH) to get the processor architecture.
So indeed, what to test? I guess we should have a test to verify that OSVersion (and OSFeatures) are not dropped
But, the main issue may be that DefaultSpec() should (probably) call Normalize() (?) after all, if the DefaultSpec() is not normalised, that's a bit odd 🤔
There was a problem hiding this comment.
I think this is a bit of a thorny area to unwind. It looks like a lot of the matcher logic does normalize passed in platform specs, but DefaultSpec() is usually used to initialize the exact tuple from client code (mostly only matters on architectures with variants, which leaves it in the realm of ARM/ARM64 at the moment).
I think we would need some analysis of all the call paths that use DefaultSpec() today to see whether losing or gaining the "vX" variant detail (depending on ARM variant/arch) would impact current expectations.
Maybe for now this test should just be a Windows test to validate we aren't losing the os.* content that was being lost prior to the PR fix which also added this test? wdyt?
|
Build succeeded.
|
The output of platforms.DefaultSpec() and the normalized version of the default platform on 32- and 64-bit ARM are not comparable. This test was added to validate not losing Windows-specific information during normalize of the platform object, so for now we are moving this to be a Windows-only test until we resolve the right behavior on ARM. Signed-off-by: Phil Estes <estesp@amazon.com>
b3bab52 to
807ded4
Compare
| @@ -0,0 +1,27 @@ | |||
| /* | |||
There was a problem hiding this comment.
Since its _test.go do we need a build directive here for windows? Or is the test tool smart enough now for *_windows_test.go to maintain the platform?
| @@ -0,0 +1,27 @@ | |||
| /* | |||
There was a problem hiding this comment.
Since its _test.go do we need a build directive here for windows? Or is the test tool smart enough now for *_windows_test.go to maintain the platform?
There was a problem hiding this comment.
It's Go magic just like the files without _test.go; noticed that we actually need to remove directives from some files as it is duplication when the filename already provides the necessary platform directive.
There was a problem hiding this comment.
Cool. I did not know that for _test files. Thanks!
There was a problem hiding this comment.
From Go cmd/go build constraints documentation:
If a file's name, after stripping the extension and a possible _test suffix, matches any of the following patterns:
*_GOOS
*_GOARCH
*_GOOS_GOARCH
(example: source_windows_amd64.go) where GOOS and GOARCH represent any known operating system and architecture values respectively, then the file is considered to have an implicit build constraint requiring those terms (in addition to any explicit constraints in the file).
There was a problem hiding this comment.
And people say golang isn't a systems language :). Super cool
|
Build succeeded.
|
|
oh! forgot about this one post-merge LGTM, thanks! |
The output of
platforms.DefaultSpec()and the normalized version of thedefault platform on 32- and 64-bit ARM are not comparable. This test
was added to validate not losing Windows-specific information during
normalize of the platform object, so for now we are moving this to be a
Windows-only test until we resolve the right behavior on ARM.
I noticed that tests on ARM64 were failing and found that #6497 added this
test; I also verified in the openlab test logs this is the new failure.
// cc: @thaJeztah
Signed-off-by: Phil Estes estesp@amazon.com