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 more testing: gRPC call failures #155

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Add more testing: gRPC call failures #155

merged 3 commits into from
Dec 9, 2024

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Nov 29, 2024

@eguzki eguzki requested a review from a team as a code owner November 29, 2024 13:59
@eguzki eguzki changed the title Add more testing Add more testing: gRPC call failures Nov 29, 2024
@eguzki
Copy link
Contributor Author

eguzki commented Nov 29, 2024

Clippy complain fixed in #154

@alexsnaps
Copy link
Member

I'd drop b805299, examples/ has some semantics in a Cargo project

@alexsnaps
Copy link
Member

Job done, so let's not throw this away, but I really think this kind of behavior should be tested for in unit tests, not solely in integration tests, these are (currently still) brittle and certainly slow given all the lifting that has to happen.

So let's work on this refactoring while we can and get the proper testing in place 🙏

@eguzki
Copy link
Contributor Author

eguzki commented Dec 2, 2024

Dropped b805299

Job done, so let's not throw this away, but I really think this kind of behavior should be tested for in unit tests, not solely in integration tests, these are (currently still) brittle and certainly slow given all the lifting that has to happen.

Agree. But working on unittests would be a waste of time right now, with a huge refactor coming ahead of us. Anyway, I thought it would be a good progress to work on moving "example" tests to proper integration tests. Specially for one good reason IMO: The integration tests must pass as they are after the refactor. So we have good better test harness than now to support the refactor.

@eguzki eguzki force-pushed the add-more-testing branch 2 times, most recently from abd2ecd to 0853af0 Compare December 2, 2024 10:08
@didierofrivia
Copy link
Member

didierofrivia commented Dec 2, 2024

so we are removing the integration e2e tests?

@alexsnaps
Copy link
Member

alexsnaps commented Dec 2, 2024

so we are removing the integration e2e tests?

That wasn't my ask and I don't think we should... but I get really confused about what we are all trying to achieve with this code base at this stage...

I'd drop b805299, examples/ has some semantics in a Cargo project

Was about that commit... i.e. do not rename it to examples as it has some specific semantic attached to it within a cargo project...

@eguzki
Copy link
Contributor Author

eguzki commented Dec 2, 2024

so we are removing the integration e2e tests?

I would not. But the desired behavior should be tested in unittests and integration tests as it fits (unittests and integration tests have different scope). However, having some "smoke" tests in e2e directory is also desirable IMO. Right now, there is one "basic" and one "remote address" e2e test. I would remove the "remote address" from that directory and move it to unittests and integration tests in following up PR's.

@eguzki
Copy link
Contributor Author

eguzki commented Dec 2, 2024

I get really confused about what we are all trying to achieve with this code base at this stage...

If you are referring to code in e2e folder, my answer would be "smoke" tests... or call it sanity tests.. Something that exercises the module running it loaded in a real gateway.

@alexsnaps
Copy link
Member

alexsnaps commented Dec 2, 2024

If you are referring to code in e2e folder

No, I was referring where you are all putting your efforts in. Based on my understanding of where things are and where we want to get them, there is work appearing that not only does not fit my understanding, but seem to only create more work for later to redo/refactor/move/... But the good news is I don't need to understand :)

So, again... all I was trying to convey is that Cargo considers ./examples/ in a special way... so don't use that name for a directory within a Cargo project other than for its intended use.

I guess @didierofrivia 's question was with regards to the stuff being deleted from e2e and "only" leaving:

./e2e
├── basic
│   ├── docker-compose.yaml
│   ├── envoy.yaml
│   ├── limits.yaml
│   ├── Makefile
│   └── README.md
├── remote-address
│   ├── docker-compose.yaml
│   ├── envoy.yaml
│   ├── limits.yaml
│   ├── Makefile
│   └── README.md
└── unreachable-service
    └── docker-compose.yaml

@eguzki
Copy link
Contributor Author

eguzki commented Dec 2, 2024

I guess @didierofrivia 's question was with regards to the stuff being deleted from e2e and "only" leaving:

The stuff being deleted from e2e has been added to integration tests in tests/failures.rs.

@eguzki
Copy link
Contributor Author

eguzki commented Dec 2, 2024

Based on my understanding of where things are and where we want to get them, there is work appearing that not only does not fit my understanding, but seem to only create more work for later to redo/refactor/move/...

That's sad. I really believed this work was in the right direction of having features tested in rust instead of heavy and slow environments as agreed. Not only valid for today, but also for later and does not create more work for later to redo/refactor/move ... 🤷

@eguzki eguzki force-pushed the add-more-testing branch 2 times, most recently from edabf77 to c785cc6 Compare December 9, 2024 14:49
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
Signed-off-by: Eguzki Astiz Lezaun <[email protected]>
@eguzki eguzki merged commit 9f462cb into main Dec 9, 2024
10 checks passed
@eguzki eguzki deleted the add-more-testing branch December 9, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants