Skip to content
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

use cabbage tests from a separate repo #1636

Merged
merged 22 commits into from
Jul 20, 2020

Conversation

ayrat555
Copy link
Contributor

@ayrat555 ayrat555 commented Jul 9, 2020

📋 Add associated issues, tickets, docs URL here.

Overview

Describe what your Pull Request is about in a few sentences.

Changes

Describe your changes and implementation choices. More details make PRs easier to review.

  • Change 1
  • Change 2
  • ...

Testing

Describe how to test your new feature/bug fix and if possible, a step by step guide on how to demo this.

Makefile Outdated
@@ -250,6 +250,9 @@ init-contracts-reorg: clean-contracts
PLASMA_FRAMEWORK_TX_HASH=$$PLASMA_FRAMEWORK_TX_HASH PLASMA_FRAMEWORK=$$PLASMA_FRAMEWORK \
PAYMENT_EIP712_LIBMOCK=$$PAYMENT_EIP712_LIBMOCK MERKLE_WRAPPER=$$MERKLE_WRAPPER ERC20_MINTABLE=$$ERC20_MINTABLE

init-integration-tests:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we git submodule perhaps?

@boolafish
Copy link
Contributor

boolafish commented Jul 10, 2020

🤔 a bit out topic of this PR. But do we want the spec to be private repo. At least the acceptance test on watcher should be public IMO.

cc. @achiurizo @T-Dnzt @kasima

@ayrat555 ayrat555 marked this pull request as ready for review July 14, 2020 12:15
@@ -581,19 +591,23 @@ jobs:
key: v2-mix-specs-cache-{{ .Branch }}-{{ checksum "mix.lock" }}
- run:
name: Print watcher logs
command: docker-compose -f docker-compose.yml -f ./priv/cabbage/docker-compose-reorg.yml -f ./priv/cabbage/docker-compose-cabbage.yml logs --follow watcher
command: docker-compose -f docker-compose.yml -f ./priv/cabbage/docker-compose-reorg.yml -f ./priv/cabbage/docker-compose-2-specs.yml logs --follow watcher
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we name this better, docker-compose-2-specs is confusing I think...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InoMurko it means the docker-compose version 2. the childchain users version 3. what do you think about docker-compose-version-2-specs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, okay... that's the reason then. perhaps there's two things:

  • should we name them by functionality?
  • should we wrap these into makefile recipes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InoMurko I moved to makefile. It doesn't have any functionality, it only overrides 2 env variables

@ayrat555 ayrat555 requested a review from InoMurko July 14, 2020 12:30
@achiurizo
Copy link
Contributor

🤔 a bit out topic of this PR. But do we want the spec to be private repo. At least the acceptance test on watcher should be public IMO.

cc. @achiurizo @T-Dnzt @kasima

Let's figure this out later once we are comfortable with how we run the specs repo then we can figure out how to open up the watcher acceptance tests there.

@boolafish
Copy link
Contributor

Why do we even wants to move to private repo now though?

I can guess why you’d like to move to separate repo but i don’t see why private yet. This change of moving it private is a degradation of what we are providing to the users now but I don’t see any discussion and justification reasoning. Would appreciate if this kinds of change can clearly list out the motivation of change in PR or have early discussion with the team 🙏

Copy link
Contributor

@InoMurko InoMurko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, but I would add some guides in the README.md so that devs know how to use this!

@ayrat555
Copy link
Contributor Author

@InoMurko I added a section about running integration tests to README

@InoMurko
Copy link
Contributor

InoMurko commented Jul 17, 2020

At least a partial reason for having these tests private is the future compliance with childchain2. Tests there will expose some of the underlying functionality we probably don't want the world to see. Or maybe we do (from the top of my head):

  • cluster failover
  • database failover
  • internal accounting (backoffice)

Also, the pain of having public and private repos is getting annoying because of potential leakage of internal issues/security stuff/new customers/...

So having a thin open presence of mainly watchers is, from my perspective, better.

@ayrat555 ayrat555 merged commit 5063f42 into master Jul 20, 2020
@ayrat555 ayrat555 deleted the use-cabbage-tests-from-separate-repo branch July 20, 2020 18:15
@unnawut unnawut added the chore Technical work that does not affect service behaviour label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Technical work that does not affect service behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants