Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

tests/e2e: introduce e2e framework basics, first test #1801

Merged
merged 15 commits into from
Oct 13, 2020

Conversation

eduser25
Copy link
Contributor

@eduser25 eduser25 commented Oct 8, 2020

  • Adds first E2E helper framework and functions
  • Adds First test utilizing the framework
  • Added README.md

Co-authored-by: Jon Huhn jon.huhn@microsoft.com

  • Tests [X]

Please answer the following questions with yes/no.
Yes

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    Yes, relevant sections have been commented with exact reference implementations that were used.

@eduser25 eduser25 added area/tests Related to tests wip Work-in-Progress labels Oct 8, 2020
@eduser25 eduser25 linked an issue Oct 8, 2020 that may be closed by this pull request
@eduser25 eduser25 requested review from nojnhuh and a team October 8, 2020 22:09
@eduser25 eduser25 force-pushed the testing-first branch 2 times, most recently from c02943e to aff9b70 Compare October 8, 2020 22:31
- Adds first E2E helper framework and functions
- Adds First test utilizing the framework
- Added README.md

Co-authored-by: Jon Huhn <jon.huhn@microsoft.com>
@eduser25 eduser25 linked an issue Oct 9, 2020 that may be closed by this pull request
3 tasks
Copy link
Contributor

@nojnhuh nojnhuh left a comment

Choose a reason for hiding this comment

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

Left a few comments inline on the README, mostly nits and clarifying kind vs. non-kind. I didn't look at the actual code assuming that hasn't changed from what we've been working off of.

Overall I'd say this is ready for review.

tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved

The framework exposes an OSM test data structure or handle, which is in effect the interaction mechanism for the test itself. The test framework takes care to collect and initialize most of the common functionalities a test would expect when deploying on K8s, including but not limited to Kubernetes and SMI clientsets, flag parsing, container registry values, cleanup hooks, etc., and provides accessibility functions through the handle for the test to use at its own discretion.

The hooks for initialization and cleanup are set at Ginkgo's `BeforeEach` at the top level of test execution (between Ginkgo `Describes`); we henceforth recommend keeping every test in its own `Describe` section, as well as on a separate file for clarity. You can refer to `suite_test.go` for more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to hear others' opinions on how best to structure the tests (at least within each top-level Describe block) and name them so they're easy to isolate.

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable. The BeforeEach etc can be within the Describe or Context containers, so it seems ok that every e2e test be in its own Describe container.

tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Show resolved Hide resolved
eduser25 and others added 6 commits October 9, 2020 11:04
Co-authored-by: Jon Huhn <nojnhuh@users.noreply.github.com>
Co-authored-by: Jon Huhn <jon.huhn@microsoft.com>
It will run separately as an individual gh action later
@codecov-io
Copy link

codecov-io commented Oct 9, 2020

Codecov Report

Merging #1801 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1801   +/-   ##
=======================================
  Coverage   59.20%   59.20%           
=======================================
  Files         125      125           
  Lines        5170     5170           
=======================================
  Hits         3061     3061           
  Misses       2106     2106           
  Partials        3        3           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1291e4b...212fb37. Read the comment docs.

SC2006: Use $(..) instead of legacy `..`.
Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Thanks for this change, will test it out.


The framework exposes an OSM test data structure or handle, which is in effect the interaction mechanism for the test itself. The test framework takes care to collect and initialize most of the common functionalities a test would expect when deploying on K8s, including but not limited to Kubernetes and SMI clientsets, flag parsing, container registry values, cleanup hooks, etc., and provides accessibility functions through the handle for the test to use at its own discretion.

The hooks for initialization and cleanup are set at Ginkgo's `BeforeEach` at the top level of test execution (between Ginkgo `Describes`); we henceforth recommend keeping every test in its own `Describe` section, as well as on a separate file for clarity. You can refer to `suite_test.go` for more details.
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable. The BeforeEach etc can be within the Describe or Context containers, so it seems ok that every e2e test be in its own Describe container.

tests/e2e/README.md Outdated Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
@eduser25 eduser25 marked this pull request as ready for review October 9, 2020 21:20
@eduser25 eduser25 removed the wip Work-in-Progress label Oct 9, 2020
@eduser25 eduser25 requested a review from a team October 9, 2020 21:20
shashankram
shashankram previously approved these changes Oct 9, 2020
Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Tested this out, looks good.

args = append(args, fmt.Sprintf("--enable-grafana=%v", instOpts.deployGrafana))
args = append(args, fmt.Sprintf("--deploy-jaeger=%v", instOpts.deployJaeger))

stdout, stderr, err := td.RunLocal(filepath.FromSlash("../../bin/osm"), args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a path to also test helm install

Copy link
Contributor

@nojnhuh nojnhuh Oct 12, 2020

Choose a reason for hiding this comment

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

Not yet, but we have that scenario documented in our running list: #1714 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, helm install should be part of the test suite regardless of the scenarios we are testing. Added #1814 to track.

shashankram
shashankram previously approved these changes Oct 12, 2020
tests/e2e/README.md Show resolved Hide resolved
tests/e2e/README.md Show resolved Hide resolved
tests/e2e/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

When running this manually, my test passed but I was confused by the output. Would you mind explaining what is happening below and why I'm seeing the REST req failed messages? and what does the nil mean here:Delete for osm-system: <nil>?

> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 0) Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req failed (status: 404) <nil>
> REST req succeeded: 200
> REST req succeeded: 200
> REST req succeeded: 200
> REST req succeeded: 200
> REST req succeeded: 200
STEP: Deleting SMI policies
> REST req failed correctly: Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed correctly: Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed correctly: Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed correctly: Remote exec err: command terminated with exit code 7 | stderr:
> REST req failed correctly: Remote exec err: command terminated with exit code 7 | stderr:
[AfterEach] [Top Level]
  /Users/michellenoorali/go/src/github.com/openservicemesh/osm/tests/e2e/suite_test.go:26
Deleting namespace client
Delete for client: <nil>
Deleting namespace server
Delete for server: <nil>
Deleting namespace osm-system
Delete for osm-system: <nil>
STEP: Waiting for namespaces [client server osm-system] to vanish
Deleting kind cluster: osm-e2e

@eduser25
Copy link
Contributor Author

I got similar feedback from @shashankram. I think we have a bit of misleading output on this first test, specifically and only because it was the first one, and we enabled a lot more verbosity on it when debugging.

We can totally improve the way it is presented to avoid misleading information and straightforward understanding.

tests/e2e/common_smi.go Outdated Show resolved Hide resolved
tests/e2e/common_smi.go Outdated Show resolved Hide resolved
@eduser25
Copy link
Contributor Author

eduser25 commented Oct 12, 2020

@michelleN This will have to do for now til we get loggers and flags to enable log verbosity for specific parts of the test.

Wait for pods ready in ns [client]...
Finished waiting for NS [client].
STEP: Creating SMI policies
STEP: [WaitForRepeatedSuccess] waiting 1m0s for 5 iterations to succeed
> (client/client -> server.server:80/) HTTP Req failed 0 Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed 0 Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed 404 <nil>
> (client/client -> server.server:80/) HTTP Req failed 404 <nil>
> (client/client -> server.server:80/) HTTP Req failed 404 <nil>
> (client/client -> server.server:80/) HTTP Req succeeded: 200
> (client/client -> server.server:80/) HTTP Req succeeded: 200
> (client/client -> server.server:80/) HTTP Req succeeded: 200
> (client/client -> server.server:80/) HTTP Req succeeded: 200
> (client/client -> server.server:80/) HTTP Req succeeded: 200
STEP: Deleting SMI policies
STEP: [WaitForRepeatedSuccess] waiting 1m0s for 5 iterations to succeed
> (client/client -> server.server:80/) HTTP Req failed correctly: Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed correctly: Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed correctly: Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed correctly: Remote exec err: command terminated with exit code 7 | stderr: 
> (client/client -> server.server:80/) HTTP Req failed correctly: Remote exec err: command terminated with exit code 7 | stderr: 
[AfterEach] [Top Level]
  /home/eserra/src/osm/tests/e2e/suite_test.go:26
Deleting namespace osm-system
Deleting namespace client
Deleting namespace server
STEP: Waiting for namespaces [osm-system client server] to vanish

@shashankram shashankram changed the title E2E first commit tests/e2e: introduce e2e testing framework Oct 12, 2020
@eduser25 eduser25 changed the title tests/e2e: introduce e2e testing framework tests/e2e: introduces e2e framework basics, first test Oct 12, 2020
@eduser25 eduser25 changed the title tests/e2e: introduces e2e framework basics, first test tests/e2e: introduce e2e framework basics, first test Oct 12, 2020
@eduser25 eduser25 merged commit 8faf189 into openservicemesh:main Oct 13, 2020
@eduser25 eduser25 deleted the testing-first branch October 13, 2020 00:58
eduser25 added a commit to eduser25/osm that referenced this pull request Oct 14, 2020
…h#1801)

* E2E first commit

- Adds first E2E helper framework and functions
- Adds First test utilizing the framework
- Added README.md

Co-authored-by: Jon Huhn <jon.huhn@microsoft.com>
draychev pushed a commit to draychev/osm that referenced this pull request Oct 28, 2020
…h#1801)

* E2E first commit

- Adds first E2E helper framework and functions
- Adds First test utilizing the framework
- Added README.md

Co-authored-by: Jon Huhn <jon.huhn@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/tests Related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create an e2e framework for osm
7 participants