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

chore(test): Integrate LDBC Tests into CI Workflow #8367

Merged
merged 23 commits into from
Nov 24, 2022

Conversation

all-seeing-code
Copy link
Contributor

@all-seeing-code all-seeing-code commented Oct 18, 2022

PR integrates LDBC tests as a CI/CD workflow.

Query descriptions can be found at https://ldbcouncil.org/ldbc_snb_docs/ldbc-snb-specification.pdf

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2022

CLA assistant check
All committers have signed the CLA.

@all-seeing-code all-seeing-code changed the title chore: Integrate LDBC Tests into CI Workflow WIP chore: Integrate LDBC Tests into CI Workflow Oct 18, 2022
Copy link
Contributor Author

@all-seeing-code all-seeing-code left a comment

Choose a reason for hiding this comment

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

Add token

@skrdgraph
Copy link
Contributor

@anurags92 - you can mark a WIP PR as draft in GH. I am marking this as draft for now, because there are other open PRs I will have to bring in.

@skrdgraph skrdgraph marked this pull request as draft October 19, 2022 21:04
@all-seeing-code all-seeing-code marked this pull request as ready for review November 22, 2022 14:38
@coveralls
Copy link

coveralls commented Nov 22, 2022

Coverage Status

Coverage increased (+0.006%) to 37.189% when pulling 8f117fa on anurag/integrate-ldbc-to-ci into bc5f584 on main.

@@ -832,6 +903,9 @@ func run() error {
if *suite == "load" || *suite == "all" {
downloadDataFiles()
}
if *suite == "ldbc" || *suite == "all" {
downloadLDBCFiles()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an expensive operation? If testing locally, maybe it should check for the existence of the LDBC files in downloadLDBCFiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does have a flag downloadResources which when passed as false will not download the resources again. This is used in load-tests as well.

Copy link
Contributor

@joshua-goldstein joshua-goldstein left a comment

Choose a reason for hiding this comment

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

PR looks good @anurags92 - only small nitpick, we made some changes to the workflow files (some cleanup). You can see the setup here.
Main differences:

  • Get Go version from .go-version file
  • Use make docker-image instead of make image-local (which automatically builds Dgraph binary, no need to call make dgraph separately, and no architecture specific arguments given)

@all-seeing-code all-seeing-code changed the title WIP chore: Integrate LDBC Tests into CI Workflow chore(test): Integrate LDBC Tests into CI Workflow Nov 23, 2022
desc := tt.Tag
// TODO(anurag): IC06 and IC10 have non-deterministic results because of dataset.
// Find a way to modify the queries to include them in the tests
if desc == "IC06" || desc == "IC10" {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Query IC06 and IC10 can have multiple correct answers based on the indexing. Will have to modify the queries to get deterministic results. Added a TODO.

Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

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

Look good. Go ahead and merge it.

@meghalims meghalims self-requested a review November 23, 2022 17:47
Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

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

Approve.

@all-seeing-code all-seeing-code merged commit 9a2427e into main Nov 24, 2022
@all-seeing-code all-seeing-code deleted the anurag/integrate-ldbc-to-ci branch November 24, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants