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

Subgraph Compositions: Validations #5770

Open
wants to merge 4 commits into
base: krishna/sgc-multiple-sg-sources-fix
Choose a base branch
from

Conversation

incrypto32
Copy link
Member

No description provided.

@incrypto32 incrypto32 added this to the Subgraph Composition milestone Jan 14, 2025
@incrypto32 incrypto32 requested a review from zorancv January 14, 2025 13:15
@@ -37,6 +37,33 @@ const GQL_SCHEMA: &str = r#"
type TestEntity @entity { id: ID! }
"#;
const GQL_SCHEMA_FULLTEXT: &str = include_str!("full-text.graphql");
// const SOURCE_SUBGRAPH_MANIFEST: &str = include_str!("source.yaml");
Copy link
Contributor

Choose a reason for hiding this comment

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

minor thing - remove the comment.

Copy link
Contributor

@zorancv zorancv left a comment

Choose a reason for hiding this comment

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

LGTM

@zorancv zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 10 times, most recently from b6f7492 to 640e7d1 Compare January 19, 2025 17:34
@incrypto32 incrypto32 force-pushed the krishna/sgc-validation branch from ee37a70 to 3944111 Compare January 20, 2025 12:35
@zorancv zorancv force-pushed the zoran/subgraph-composition-rework-vid-wrap2 branch 3 times, most recently from 14671db to 640e7d1 Compare January 21, 2025 15:24
@@ -1091,54 +1090,6 @@ async fn parse_data_source_context() {
);
}

#[tokio::test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking a second look at this PR, why is this test removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons:

  1. The test is redudant - Everything this test does is done by the integraiton test
  2. The test keeps failing because it requires an actual source subgraph.. and some refactoring of the code is required to actually deploy a dummy source subgraph just for the validation to work. But in reality in runner test we mock the triggers and push the triggers directly and no source subgraph is actually needed.

SO i felt its just better to remove this test since, everything this test does is already done by the integration test and the work required to just make the new valdiations work with runner tests is probably unneccessary considering 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Thanks for the explanation!

@zorancv zorancv assigned zorancv and incrypto32 and unassigned zorancv Jan 23, 2025
@incrypto32 incrypto32 force-pushed the krishna/sgc-validation branch 2 times, most recently from ae35584 to 799a3e9 Compare January 28, 2025 11:56
@incrypto32 incrypto32 changed the base branch from zoran/subgraph-composition-rework-vid-wrap2 to krishna/sgc-multiple-sg-sources-fix January 28, 2025 11:59
@incrypto32 incrypto32 force-pushed the krishna/sgc-multiple-sg-sources-fix branch from 06edc2b to f38680e Compare January 28, 2025 12:08
@incrypto32 incrypto32 force-pushed the krishna/sgc-validation branch from 799a3e9 to b98760f Compare January 28, 2025 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants