Implemented skip unsafe flag for our integration tests

[miladz68]

This change is 

[dzmitryhil]

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

---

integration-tests/init.go line 56 at r1 (raw file):

	flag.StringVar(&logFormat, "log-format", string(logger.ToolDefaultConfig.Format), "Format of logs produced by tests")
	flag.StringVar(&chainID, "chain-id", string(constant.ChainIDDev), "Which chain-id to use (coreum-devnet-1, coreum-testnet-1,...)")
	flag.BoolVar(&skipUnsafe, "skip-unsafe", true, "skip unsafe tests for example ones related to governance")

But how do we check that the flag is required?

[dzmitryhil]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68, @silverspase, @wojtek-coreum, and @ysv)

[miladz68]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)

---

integration-tests/init.go line 56 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

But how do we check that the flag is required?

we don't check that it is required, but we default it true and rely on crust to ensure that it always false if you use crust (in CI). the default flag package does not have a method to ensure a flag is provided and we need to process cmd args manually to make sure it is provided which is a little ugly.

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase, @wojtek-coreum, and @ysv)

[ysv]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)

a discussion (no related file):
Have you 100% checked all tests ?

---

---

integration-tests/chain.go line 171 at r1 (raw file):

// SetSkipUnsafe sets skip unsafe flag.
func (c *Chain) SetSkipUnsafe(skipUnsafe bool) {

I disagree that it should be a method & field of chain
I see a better place for it: put this field into global variable cfg

and we can just make it function not method

func SkipUnsafe(t *testing.T) {
  if cfg.skipUnsafe {
   t.SkipNow()
  }
}

[miladz68]

Reviewable status: 8 of 10 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, @wojtek-coreum, and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

Have you 100% checked all tests ?

I have.

---

---

integration-tests/chain.go line 171 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I disagree that it should be a method & field of chain
I see a better place for it: put this field into global variable cfg

and we can just make it function not method

func SkipUnsafe(t *testing.T) {
  if cfg.skipUnsafe {
   t.SkipNow()
  }
}

Done.

[wojtek-coreum]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @miladz68, @silverspase, and @ysv)

a discussion (no related file):
I would say that it should be done the other way. Unsafe tests should be skipped by default and executed only if --unsafe flag is provided

---

[ysv]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

I would say that it should be done the other way. Unsafe tests should be skipped by default and executed only if --unsafe flag is provided

This has already been discussed
I had opinion similar to yours and Milad & Dzmitry suggested current behaviour

Don't have strong opinion here. Could be discussed again if needed

---

---

integration-tests/chain.go line 172 at r2 (raw file):

// unsafe tests can only be run against a locally running chain since they modify parameters
// of the chain.
func (c Chain) SkipUnsafe(t *testing.T) {

doesn't make sense for me to make it a method of chain
just usual func is ok

side benefit is that you can call it right in the beginning of func then before initialization of chain variable

[miladz68]

Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @silverspase, @wojtek-coreum, and @ysv)

---

integration-tests/chain.go line 172 at r2 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

doesn't make sense for me to make it a method of chain
just usual func is ok

side benefit is that you can call it right in the beginning of func then before initialization of chain variable

Done.

[wojtek-coreum]

Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @dzmitryhil, @silverspase, and @ysv)

a discussion (no related file):

Previously, ysv (Yaroslav Savchuk) wrote…

This has already been discussed
I had opinion similar to yours and Milad & Dzmitry suggested current behaviour

Don't have strong opinion here. Could be discussed again if needed

I think in 99% of cases boolean flag is used to set sth to true, not to false.

---

[dzmitryhil]

Reviewed 1 of 2 files at r2, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @ysv)

[ysv]

Reviewed 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase)

[miladz68]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)

a discussion (no related file):

Previously, wojtek-coreum (Wojtek) wrote…

I think in 99% of cases boolean flag is used to set sth to true, not to false.

@ysv how about we call the flag run-unsafe and keep the default value to false ?

---

[wojtek-coreum]

Reviewed 1 of 2 files at r2, 9 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @miladz68 and @silverspase)

---

.github/workflows/ci.yml line 19 at r5 (raw file):

          "build", 
          "integration tests coreum-modules", 
          # "integration tests coreum-upgrade", 

why is it needed?

---

integration-tests/chain.go line 175 at r5 (raw file):

// unsafe tests can only be run against a locally running chain since they modify parameters
// of the chain.
func SkipUnsafe(t *testing.T) {

chain.go is not the best file for this function

[miladz68]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @silverspase and @wojtek-coreum)

---

.github/workflows/ci.yml line 19 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

why is it needed?

upgrade test is considered an unsafe test and we skip it, it means the upgrade action never takes place and the test fails.

---

integration-tests/chain.go line 175 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

chain.go is not the best file for this function

none of the other files are good as well tbh.

[miladz68]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @silverspase and @wojtek-coreum)

---

.github/workflows/ci.yml line 19 at r5 (raw file):

Previously, miladz68 (milad) wrote…

upgrade test is considered an unsafe test and we skip it, it means the upgrade action never takes place and the test fails.

we can enable it back after crust is integrated to run unsafe tests.

[wojtek-coreum]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @silverspase)

---

integration-tests/chain.go line 175 at r5 (raw file):

Previously, miladz68 (milad) wrote…

none of the other files are good as well tbh.

You are right, let's create a new one like testing.go

[miladz68]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/chain.go line 175 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

You are right, let's create a new one like testing.go

but that is not a very informative name, how about helpers.go ?

[wojtek-coreum]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @silverspase)

---

integration-tests/chain.go line 175 at r5 (raw file):

Previously, miladz68 (milad) wrote…

but that is not a very informative name, how about helpers.go ?

entropies of both names are equal :D. Both are ok for me.

[dzmitryhil]

Reviewed 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68, @silverspase, and @wojtek-coreum)

[wojtek-coreum]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase)

[miladz68]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)

---

integration-tests/chain.go line 175 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

entropies of both names are equal :D. Both are ok for me.

Done.

[ysv]

Reviewed 1 of 1 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @silverspase and @wojtek-coreum)

a discussion (no related file):
run-unsafe makes a bit more sense for me so agree with Wojtek a bit more

I think in 99% of cases boolean flag is used to set sth to true, not to false.
Because of this mostly

Still not critical and both options are acceptable for me

---

[wojtek-coreum]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase)

[dzmitryhil]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase)

[ysv]

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @silverspase)