Testing profiles will be chosen based on test groups

[miladz68]

This change is 

[ysv]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dzmitryhil, @miladz68, @vertex451, and @wojtek-coreum)

---

cmd/znet/main.go line 107 at r1 (raw file):

			case lo.Some(configF.TestGroups, []string{apps.TestGroupCoreumIBC, apps.TestGroupCoreumUpgrade}):
				configF.Profiles = []string{apps.ProfileIntegrationTestsIBC}
			case len(configF.TestGroups) == 0:

minor: 1st & 2nd case could be merged

---

infra/apps/profiles.go line 79 at r1 (raw file):

// BuildAppSet builds the application set to deploy based on provided profiles.
func BuildAppSet(appF *Factory, profiles []string, coredVersion string) (infra.AppSet, error) {

IMO this func start to get complicated in terms of business logic.
Also I'm not 100% sure how it behaves for some cases
WDYT about adding unit-tests for it ?

---

infra/apps/profiles.go line 103 at r1 (raw file):

		}

		pMap[ProfileFaucet] = true

do you really need faucet in case of coreum-modules tests ?

---

infra/apps/profiles.go line 106 at r1 (raw file):

	}

	if pMap[ProfileIntegrationTestsIBC] {

I thought that idea is not to set one profile based on another.
IMO ProfileIntegrationTestsIBC standalone profile and instead of setting IBC you should treat it like this:

	if pMap[ProfileIBC] ||  pMap[ProfileIntegrationTestsIBC] {
		appSet = append(appSet, appF.IBC(AppPrefixIBC, coredApp)...)
	}

Also it seems to be more clear

[miladz68]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @dzmitryhil, @vertex451, @wojtek-coreum, and @ysv)

---

cmd/znet/main.go line 107 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

minor: 1st & 2nd case could be merged

Done

---

infra/apps/profiles.go line 79 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

IMO this func start to get complicated in terms of business logic.
Also I'm not 100% sure how it behaves for some cases
WDYT about adding unit-tests for it ?

the code is not written in a testable way, do you want a major refactor ?

---

infra/apps/profiles.go line 103 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

do you really need faucet in case of coreum-modules tests ?

Well the code is not written in a testable way, do you want a major refactor ?

---

infra/apps/profiles.go line 106 at r1 (raw file):

Previously, ysv (Yaroslav Savchuk) wrote…

I thought that idea is not to set one profile based on another.
IMO ProfileIntegrationTestsIBC standalone profile and instead of setting IBC you should treat it like this:

	if pMap[ProfileIBC] ||  pMap[ProfileIntegrationTestsIBC] {
		appSet = append(appSet, appF.IBC(AppPrefixIBC, coredApp)...)
	}

Also it seems to be more clear

well, the main idea was to decide which profiles to start based on the test-groups flags.

[dzmitryhil]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @vertex451, @wojtek-coreum, and @ysv)

[dzmitryhil]

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

---

infra/apps/profiles.go line 103 at r1 (raw file):

Previously, miladz68 (milad) wrote…

Well the code is not written in a testable way, do you want a major refactor ?

I see that @miladz68 just update the constants, and partially touched the existing code, not sure that he should add tests for it. But let's discuss.

[miladz68]

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

---

infra/apps/profiles.go line 103 at r1 (raw file):

Previously, dzmitryhil (Dzmitry Hil) wrote…

I see that @miladz68 just update the constants, and partially touched the existing code, not sure that he should add tests for it. But let's discuss.

Discussed that we don't need it.

---

infra/apps/profiles.go line 106 at r1 (raw file):

Previously, miladz68 (milad) wrote…

well, the main idea was to decide which profiles to start based on the test-groups flags.

discussed that we keep it as is.

[dzmitryhil]

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

[ysv]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @vertex451 and @wojtek-coreum)

[wojtek-coreum]

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @miladz68 and @vertex451)

---

infra/apps/profiles.go line 82 at r3 (raw file):

func BuildAppSet(appF *Factory, profiles []string, coredVersion string) (infra.AppSet, error) {
	pMap := map[string]bool{}
	if err := checkProfiles(pMap); err != nil {

the fact that pMap is modified inside this function is not obvious. It would be better to return the map

---

infra/apps/profiles.go line 174 at r3 (raw file):

}

func decideNumOfCoredValidators(pMap map[string]bool) int {

this function is not used

[miladz68]

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

---

infra/apps/profiles.go line 82 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

the fact that pMap is modified inside this function is not obvious. It would be better to return the map

Done.

---

infra/apps/profiles.go line 174 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

this function is not used

Done.

[ysv]

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

[wojtek-coreum]

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

---

cmd/znet/main.go line 113 at r5 (raw file):

			}
			if lo.Contains(configF.TestGroups, apps.TestGroupFaucet) {
				configF.Profiles = []string{apps.ProfileFaucet}

I think it should append, no?

---

infra/apps/profiles.go line 82 at r3 (raw file):

Previously, miladz68 (milad) wrote…

Done.

I mean you may create this map internally in the function, and not pass as a parameter here

---

infra/apps/profiles.go line 174 at r5 (raw file):

		return 5
	default:
		return 1

maybe it should panic?

[miladz68]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @vertex451, @wojtek-coreum, and @ysv)

---

cmd/znet/main.go line 113 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I think it should append, no?

Done.

---

infra/apps/profiles.go line 82 at r3 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

I mean you may create this map internally in the function, and not pass as a parameter here

gotcha, Done

---

infra/apps/profiles.go line 174 at r5 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

maybe it should panic?

Done

[wojtek-coreum]

Reviewed 1 of 2 files at r6, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @miladz68, @vertex451, and @ysv)

---

infra/apps/profiles.go line 178 at r6 (raw file):

func checkProfiles(profiles []string) (map[string]bool, error) {
	var pMap map[string]bool

pMap := map[string]bool{} otherwise it will panic

[wojtek-coreum]

Reviewed 1 of 2 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @miladz68 and @vertex451)

[miladz68]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @vertex451 and @wojtek-coreum)

---

infra/apps/profiles.go line 178 at r6 (raw file):

Previously, wojtek-coreum (Wojtek) wrote…

pMap := map[string]bool{} otherwise it will panic

Done.

[wojtek-coreum]

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