-
Notifications
You must be signed in to change notification settings - Fork 673
[CI]: rewrite compose test #4359
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
Conversation
2db7f76
to
9b566d7
Compare
9b566d7
to
46e50ee
Compare
Rebased on principles. |
Unrelated failures: |
helpers.Ensure("compose", "-f", data.Temp().Path("project-1", "compose.yaml"), "up", "-d") | ||
helpers.Ensure("compose", "-f", data.Temp().Path("project-2", "compose.yaml"), "up", "-d") | ||
helpers.Ensure("compose", "-f", data.Temp().Path("project-2", "compose.yaml"), "down", "-v") | ||
helpers.Ensure("compose", "-f", data.Temp().Path("project-2", "compose.yaml"), "up", "-d") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call data.Temp()
multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data.Temp()
is just an accessor.
Alternatively we could hold a ref to the returned value, but that would probably be exactly the same thing as the compiler should inline the function call.
Anyhow, if you prefer it changed to call once and keep a ref on it for readability, that is fine with me of course, so, just lmk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I have always been curious what the compared cost of a function call versus having variables moved to the heap (I assume the later is much more costly).
(sometimes I wish I would know more about how a compiler works...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the compiler is smart enough to detect that Temp()
is idempotent and safe to inlinize it.
I'd still prefer this to be called just once, but no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting conversation.
The compiler will inline this (which is the pattern used here):
package main
type Bar struct {
}
type Foo struct {
pv *Bar
}
func (f *Foo) PV() *Bar {
return f.pv
}
func New() *Foo {
return &Foo{pv: &Bar{}}
}
func main() {
f := New()
f.PV()
f.PV()
f.PV()
}
./main.go:20:6: inlining call to (*Foo).PV
./main.go:21:6: inlining call to (*Foo).PV
./main.go:22:6: inlining call to (*Foo).PV
Whether this happens in the context of tests
is an open question though.
I'd still prefer this to be called just once, but no strong opinion.
Sure.
Let me do it now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest push has the change ^.
46e50ee
to
854bc39
Compare
Rebased. |
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
854bc39
to
53e69a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Tentatively fix #4146