test: integration build the ignite binary #2639
Conversation
Previously, the ignite binary was supposed to be present in the path, which can be error prone if the binary is not up to date with the integration version. The change builds the binary before the integration are run, using a init() function. This is not very adequate since the binary is not removed at the end of the test. To be able to remove it at the end of the tests we could: 1) add a TestMain in each integration sub-package: not super fan of this because it involves to repeat the same code for each package 2) move all tests in the same package: preferable but delete the separation between the tests.
tbruyelle
requested review from
fadeev,
ilgooz,
ivanovpetr,
jeronimoalbi and
lumtis
as code owners
|
I think moving the tests to the same package is fine. We will still retain the separation of the modules themselves - the new test module might just be very large. |
I don't get that point. How this would retain the separation of the modules ? |
I just mean that if we include tests in a separate packages ("pkg_test") we can still separate the logic of the modules themselves. This _test package can just import them. Maybe I'm misunderstanding something though lol |
|
This makes a lot of sense! It fell into oblivion because we usually let the CI run integration tests since they are way slow to run a user machine -can drive one crazy Haha |
integration/env.go
Outdated
| IgniteApp string | ||
| ) | ||
|
|
||
| func init() { |
There was a problem hiding this comment.
Can we avoid using init, maybe not that important for this case but I usually find it causing side effects.
I propose we:
- Create a package level func that does the installation, named as
InstallBinary()with...Optionss if necessary. - Store a
sync.Oncein a global variable:var installBinaryOnce sync.Once - And add logic into
Newto callinstallBinaryOncewithInstallBinary.
There was a problem hiding this comment.
Done here e8dcbdb
integration/env.go
Outdated
| panic(err) | ||
| } | ||
| IgniteApp = path.Join(tmp, "ignite") | ||
| command := exec.Command("go", "build", |
There was a problem hiding this comment.
Does it make sense to use this here: https://github.com/ignite/cli/blob/develop/ignite/pkg/gocmd/gocmd.go
There was a problem hiding this comment.
good point e8dcbdb
integration/env.go
Outdated
| } | ||
| IgniteApp = path.Join(tmp, "ignite") | ||
| command := exec.Command("go", "build", | ||
| "-o", IgniteApp, appPath+"/ignite/cmd/ignite") |
There was a problem hiding this comment.
Can we define this path in a const defined in the beginning of the file?
Also: - use sync.Once rather than init() func - use gocmd rather than exec.Command directly
tbruyelle
force-pushed
the
test/integration-build-binary
branch
from
d391ed4 to
e8dcbdb
Compare
|
I finally changed my mind, I was focused on cleaning the compiled binary at the end of tests, but as I said it requires to centralize all the integration tests in the same package. This can be annoying because those tests are long to run and it's actually nice to run a subset of tests by passing a specific sub-package to the go test command. So the binary will be always located at |
tbruyelle
changed the title
|
conflicts |
@aljo242 fixed thx! |
* test: integration build the ignite binary (WIP) Previously, the ignite binary was supposed to be present in the path, which can be error prone if the binary is not up to date with the integration version. The change builds the binary before the integration are run, using a init() function. This is not very adequate since the binary is not removed at the end of the test. To be able to remove it at the end of the tests we could: 1) add a TestMain in each integration sub-package: not super fan of this because it involves to repeat the same code for each package 2) move all tests in the same package: preferable but delete the separation between the tests. * test: constant ignite binary location Also: - use sync.Once rather than init() func - use gocmd rather than exec.Command directly * chore: no longer need to build the binary in gh action * change cl section Co-authored-by: Alex Johnson <alex@shmeeload.xyz>
Previously, the ignite binary was supposed to be present in the path,
which can be error prone if the binary is not up to date with the
integration version.
The change builds the binary before the integration are run, using a
init() function. This is not very adequate since the binary is not
removed at the end of the test. To be able to remove it at the end of
the tests we could:
because it involves to repeat the same code for each package
separation between the tests.
If someone could give me a hint on this, it would be nice!
Fix #2635