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

Implement Local Hub integration tests as separate job #683

Merged
merged 11 commits into from
Mar 30, 2022

Conversation

mkuziemko
Copy link

Description

Changes proposed in this pull request:

  • moves existing Local Hub tests out of our E2E integration tests suite,
  • use test setup with Neo4j + Hub JS + test delegated storage + run Go tests using docker-compose
  • move and refactor tests from @mszostok's PRs,
  • modify GitHub Actions pipeline and run these tests in parallel to our full E2E integration tests

Testing

  1. Checkout to this PR
  2. Run make test-localhub in order to run local Hub tests - it will create the env and run the tests.

You can check the example execution of the tests in Github Action in my fork repository.

Related issue(s)

@pkosiec
Copy link
Member

pkosiec commented Mar 22, 2022

@mkuziemko I didn't check this PR yet, but I can see that you modified only the PR build. Please also make changes to main branch pipeline and put a link to example run on your fork's main branch in the PR description.

@pkosiec pkosiec self-assigned this Mar 23, 2022
@pkosiec pkosiec added enhancement New feature or request area/hub Relates to Hub area/ci Relates to CI labels Mar 23, 2022
Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

Good work, it works well! 🚀 I have a few comments regarding these moved tests or some cosmetic stuff, like naming 🙂

.github/workflows/branch-build.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
hub-js/graphql/local/schema.graphql Outdated Show resolved Hide resolved
test/e2e/hub_test.go Outdated Show resolved Hide resolved
test/localhub/docker-compose.yml Outdated Show resolved Hide resolved
test/localhub/docker-compose.yml Outdated Show resolved Hide resolved
test/localhub/external_storage_backend_test.go Outdated Show resolved Hide resolved
test/localhub/docker-compose.yml Outdated Show resolved Hide resolved
test/localhub/docker-compose.yml Outdated Show resolved Hide resolved
test/localhub/lock_test.go Outdated Show resolved Hide resolved
hack/test-localhub.sh Outdated Show resolved Hide resolved
@mkuziemko mkuziemko added the WIP Work in progress label Mar 25, 2022
@mkuziemko mkuziemko force-pushed the add_localhub_tests branch 3 times, most recently from 274051d to 4fd26fe Compare March 25, 2022 12:39
@mkuziemko mkuziemko force-pushed the add_localhub_tests branch from 4fd26fe to 7ca8f46 Compare March 25, 2022 12:59
@mkuziemko mkuziemko removed the WIP Work in progress label Mar 25, 2022
Makefile Show resolved Hide resolved
test/local-hub/lock_test.go Outdated Show resolved Hide resolved
test/local-hub/lock_test.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
"$id": "#/properties/contextSchema/properties/name",
"type": "string",
"enum": [
"aws_secretsmanager",
Copy link
Member

Choose a reason for hiding this comment

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

🤔 Why do the tests pass for creating TypeInstances without context, if our schema says it is required? We do validate the context, right? Is your PR rebased?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, if we create the TypeInstance without context then the default context is used (for use is the dotenv). Here is a place where it is checked.

@mkuziemko mkuziemko force-pushed the add_localhub_tests branch from 84d50a0 to 149868f Compare March 28, 2022 16:56
Copy link
Member

@pkosiec pkosiec left a comment

Choose a reason for hiding this comment

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

LGTM - just a few last comments. Please review them before merge 🙂

test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
t.Helper()
url := strings.Split(addr, ":")
assert.Equal(t, len(url), 2)
conn, err := grpc.Dial(fmt.Sprintf("[%s]:%s", url[0], url[1]), grpc.WithInsecure())
Copy link
Member

@pkosiec pkosiec Mar 29, 2022

Choose a reason for hiding this comment

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

Questions:

  1. Why do we have square brackets in the URL? What does it do?
  2. Can we use the addr there directly?

EDIT: Alright, it is for supporting IPv6 addresses: https://grpc.github.io/grpc/core/md_doc_naming.html
However, I'm not really sure if we need that 😄 We can save a few lines and use addr directly. But it's up to you, we can keep it 🙂

test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
test/local-hub/external_storage_backend_test.go Outdated Show resolved Hide resolved
@mkuziemko mkuziemko merged commit 45936bf into capactio:main Mar 30, 2022
@mkuziemko mkuziemko deleted the add_localhub_tests branch March 30, 2022 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Relates to CI area/hub Relates to Hub enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants