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

Add e2e tests of a complex service #1500

Merged
merged 17 commits into from
Nov 22, 2019
Merged

Add e2e tests of a complex service #1500

merged 17 commits into from
Nov 22, 2019

Conversation

krhubert
Copy link
Contributor

More complex service test for #1446

@krhubert krhubert added the tests label Nov 14, 2019
@krhubert krhubert added this to the next milestone Nov 14, 2019
@krhubert krhubert self-assigned this Nov 14, 2019
@krhubert krhubert mentioned this pull request Nov 14, 2019
@NicolasMahe
Copy link
Member

Why not implementing the complex service directly in the current e2e test service?

@krhubert
Copy link
Contributor Author

krhubert commented Nov 15, 2019

To split the tests services and not put everthing into one. You test also deploying multiple services and we still might use them to test process. Also as you can see there was a pbm with deleting 2nd of them which wouldn't came up when I would implement that in one service.


service, err := client.ServiceClient.Get(ctx, &pb.GetServiceRequest{Hash: testServiceHash})
require.NoError(t, err)
want := pb.TransformCreateReqToService(req)
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this protobuf.TransformCreateReqToService.
The hash calculation should be done by the service sdk.
And I don't think it makes sense to check if the hash is expected. This is a e2e tests, not a unit test.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to revert this function and not check the expected service hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok we don't need to check it. But TransformCreateReqToService should there, but maybe without hash calculation. Sdk pachage shoudn't have logic how transform pb structures

Copy link
Member

Choose a reason for hiding this comment

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

With the discussion of the last meeting, we spoke about bringing the requests closer to the sdk..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The request yes, but this is transformation and it's different.

e2e/definition_test.go Outdated Show resolved Hide resolved
@krhubert
Copy link
Contributor Author

krhubert commented Nov 19, 2019

@NicolasMahe I've tried to debug it without success, can you take a look on that for few mins?

@NicolasMahe
Copy link
Member

NicolasMahe commented Nov 20, 2019

@NicolasMahe I've tried to debug it without success, can you take a look on that for few mins?

I improved the test and revert c0ab023 but could not figure how to fix this issue. So the test is now skipped with a FIXME message.

The problem occurs only on the CI and only with a service with at least one dependency. It looks like the dependency is server killed and thus the service is never stopped. I will create an issue and investigate later as it is not blocking.

@NicolasMahe NicolasMahe merged commit 0d1427f into dev Nov 22, 2019
@NicolasMahe NicolasMahe deleted the test/e2e-complex-service branch November 22, 2019 05:03
@NicolasMahe NicolasMahe added the release:add Pull requests that add something label Nov 26, 2019
@NicolasMahe NicolasMahe changed the title Test/e2e complex service Add e2e tests of a complex service Nov 26, 2019
@NicolasMahe NicolasMahe mentioned this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:add Pull requests that add something tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants