-
Notifications
You must be signed in to change notification settings - Fork 132
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 integration tests project #1155
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
bd8390d
to
fb554bc
Compare
fb554bc
to
d5bd407
Compare
@@ -23,6 +23,7 @@ members = [ | |||
"translator", | |||
"jd-client", | |||
"jd-server", | |||
"tests-integration", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe there is a better name.. roles-integration
is other option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be misleading? or can be roles-integration-tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I also prefer roles-integration-tests
. Also in the relative Cargo.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should just be tests
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
initially tests-integration
was chosen because there is already a tests-utils
folder and I wanted to be consistent. Is it worth deciding on the folder naming now, or when we know better how the role's crates are going to be structured/named? I prefer to keep it as is and rename it when restructuring the roles
crates is underway, otherwise happy to change to either tests
or roles-integration-tests
, whatever gets more thumbs up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup does not follow Rust's way of doing integration tests. See my comments on #1120:
If the intention of this PR is to setup the basic scaffolding of these tests, we should follow the aforementioned patterns.
Thanks for the review! Do you have an example repo with the mentioned structure for 'Multiple crate integration test setup'? |
I agree with the sentiment against adding yet another crate to the codebase. However, given the fact that all roles are already separate crates under the same workspace, I'm afraid this is the only available path forward. If we just added a I tried implementing this locally and got stuck with:
Maybe in the future when we have a unified crate containing all roles (which is not something we're aiming for now), it will be easier to bypass the need for a new crate and we will be able to have all integration tests as part of the same crate (much like For now, I could live with this tradeoff of having one extra crate inside |
Ahh that is too bad. Let's do what ldk-node does then. |
I think we should consider having integration tests in each |
Yup that would be great to have. But as mentioned, its really outside of the scope of this Pull Request. |
863cf29
to
6eb932a
Compare
Adds an initial integration tests project to the `roles` workspace. This crate should hold tests related to the other crates in the `roles` workspace, allowing us to write integration tests for those.
6eb932a
to
633684a
Compare
Bencher
🚨 14 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK
Adds an initial integration tests project to the
roles
workspace.This crate should hold tests related to the other crates in the
roles
workspace, allowing us to write integration tests for those.