-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_integration_tests: clean old todos #11502
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
apollo_integration_tests: clean old todos #11502
Conversation
Itay-Tsabary-Starkware
left a comment
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.
@Itay-Tsabary-Starkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @meship-starkware, and @TzahiTaub).
crates/apollo_integration_tests/tests/common/mod.rs line 4 at r1 (raw file):
// This means that any peace of code in this module that is not used by *all* test modules will be // identified as unused code by clippy (for one of the crates). #![allow(dead_code)]
Why do we have a tests/common/mod.rs module here?
Are these utilities used by all/some of the integration tests? We can simply move them to src/utils then, the entire purpose of this crate is testing, so we don't need to go the extra mile of separating testing utils into a hacky tests/src-code-stuff hierarchy.
@ArniStarkware could you add a todo for that?
Code quote:
// Each test module is compiled as a separate crate, and all can declare the common module.
// This means that any peace of code in this module that is not used by *all* test modules will be
// identified as unused code by clippy (for one of the crates).
#![allow(dead_code)]ad33649 to
7a3abbf
Compare
ee26577 to
d69d8d3
Compare
ArniStarkware
left a comment
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.
@ArniStarkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware, @meship-starkware, and @TzahiTaub).
crates/apollo_integration_tests/tests/common/mod.rs line 4 at r1 (raw file):
Previously, Itay-Tsabary-Starkware wrote…
Why do we have a
tests/common/mod.rsmodule here?
Are these utilities used by all/some of the integration tests? We can simply move them tosrc/utilsthen, the entire purpose of this crate is testing, so we don't need to go the extra mile of separating testing utils into a hackytests/src-code-stuffhierarchy.@ArniStarkware could you add a todo for that?
Done.
meship-starkware
left a comment
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.
@meship-starkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware, @Itay-Tsabary-Starkware, and @TzahiTaub).
crates/apollo_integration_tests/tests/end_to_end_flow_test.rs line 18 at r2 (raw file):
mod common; // TODO(Meshi): Fail the test if no class have migrated.
Would the test fail if the migration fails? I am not sure if this is done.
Code quote:
// TODO(Meshi): Fail the test if no class have migrated.
ArniStarkware
left a comment
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.
@ArniStarkware made 1 comment.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @meship-starkware, and @TzahiTaub).
crates/apollo_integration_tests/tests/end_to_end_flow_test.rs line 18 at r2 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Would the test fail if the migration fails? I am not sure if this is done.
I will bring this TODO back and move this todo in this PR: 11504
meship-starkware
left a comment
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.
@meship-starkware resolved 1 discussion.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware and @TzahiTaub).
meship-starkware
left a comment
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.
@meship-starkware reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware and @TzahiTaub).
Itay-Tsabary-Starkware
left a comment
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.
@Itay-Tsabary-Starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub).
d69d8d3 to
7e99845
Compare
ArniStarkware
left a comment
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.
@ArniStarkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub).
d8f486b to
500ffab
Compare
500ffab to
5ed453a
Compare
75fcebd

No description provided.