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.

Sign up for GitHub

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

Jump to bottom

test(upgrade): add upgrade tests for 21million dataset #8927

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

test(upgrade): add upgrade tests for 21million dataset #8927

wants to merge 9 commits into from

Conversation

jbhamra1
Copy link
Contributor

@jbhamra1 jbhamra1 commented Aug 2, 2023 •
edited by mangalaman93

@dgraph-bot dgraph-bot added area/testing Testing related issues go Pull requests that update Go code labels Aug 2, 2023
@jbhamra1 jbhamra1 force-pushed the jassi/systest_21million branch 2 times, most recently from 098f872 to 352f44b Compare August 4, 2023 18:00
@mangalaman93 mangalaman93 changed the title Changes for Upgrade tests for systest/21million test(upgrade): add upgrade tests for 21million dataset Aug 7, 2023
@jbhamra1 jbhamra1 force-pushed the jassi/systest_21million branch 3 times, most recently from 8acbe18 to 8c90797 Compare August 14, 2023 08:11
mangalaman93
mangalaman93 requested changes Aug 16, 2023
View reviewed changes
dgraphtest/config.go Outdated Show resolved Hide resolved
dgraphtest/local_cluster.go Outdated Show resolved Hide resolved
dgraphtest/local_cluster.go Outdated Show resolved Hide resolved
@CLAassistant
Copy link

CLAassistant commented Aug 17, 2023 •
edited

CLA assistant check
All committers have signed the CLA.

@jbhamra1 jbhamra1 force-pushed the jassi/systest_21million branch 3 times, most recently from 519fc7e to 822ccdd Compare August 20, 2023 16:48
@jbhamra1 jbhamra1 marked this pull request as ready for review August 21, 2023 08:23
mangalaman93
mangalaman93 requested changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Also, you probably want to remove downloading 21million dataset in the t.go

dgraphtest/local_cluster.go Outdated Show resolved Hide resolved
dgraphtest/local_cluster.go Outdated Show resolved Hide resolved
dgraphtest/local_cluster.go Show resolved Hide resolved
dgraphtest/secrets/secret-key Outdated Show resolved Hide resolved
ee/acl/acl_test.go Outdated Show resolved Hide resolved
systest/21million/bulk/upgrade_test.go Outdated Show resolved Hide resolved
systest/21million/live/integration_test.go Outdated Show resolved Hide resolved
systest/21million/live/run_test.go Show resolved Hide resolved
systest/bgindex/count_test.go Outdated Show resolved Hide resolved
systest/21million/live/run_test.go Outdated
func cleanupAndExit(exitCode int) {
_ = os.RemoveAll("./t")
os.Exit(exitCode)
func downloadDataFiles(testDir string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also defined twice

@jbhamra1 jbhamra1 force-pushed the jassi/systest_21million branch 5 times, most recently from 4a718be to 031aa56 Compare August 23, 2023 13:44
@jbhamra1 jbhamra1 force-pushed the jassi/systest_21million branch 2 times, most recently from d0f19b8 to 2d7c5fb Compare August 25, 2023 06:16
mangalaman93
mangalaman93 requested changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@mangalaman93 mangalaman93 left a comment

Choose a reason for hiding this comment

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

Just a few minor comments. Looks pretty good otherwise.

systest/21million/bulk/upgrade_test.go
dc dgraphtest.Cluster
lc *dgraphtest.LocalCluster
uc dgraphtest.UpgradeCombo
bulkDataDir string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could leave an empty line before this so that the spacing looks better.

systest/21million/common/run_queries.go Outdated
filename := filepath.Join(queryDir, file.Name())
reader, cleanup := chunker.FileReader(filename, nil)
bytes, err := io.ReadAll(reader)
if err != nil {
t.Fatalf("Error reading file: %s", err.Error())
panic("error reading file")
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we not returning an error here?

systest/21million/common/run_queries.go Outdated
@@ -84,20 +89,27 @@ func TestQueriesFor21Million(t *testing.T) {
}

if ctx.Err() == context.DeadlineExceeded {
t.Fatal("aborting test due to query timeout")
panic("aborting test due to query timeout")
Copy link
Contributor

Choose a reason for hiding this comment

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

same

systest/21million/common/run_queries.go Outdated
testutil.CompareJSON(t, bodies[1], string(resp.GetJson()))
log.Printf("running %s", file.Name())
if err = dgraphtest.CompareJSON(bodies[1], string(resp.GetJson())); err != nil {
log.Println(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

we should return an error here too

systest/21million/common/run_queries.go Outdated

func QueriesFor21Million(dc dgraphtest.Cluster) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

QueriesFor21Million -> RunTestQueriesFor21Million
and I think we should pass t object to this function so that we could do t.Run inside.

systest/21million/live/upgrade_test.go
dc dgraphtest.Cluster
lc *dgraphtest.LocalCluster
uc dgraphtest.UpgradeCombo
liveDataDir string
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: same here, can leave a newline before this

dgraphtest/load.go Outdated
if gr, err := gzip.NewReader(r); err != nil &&
strings.Contains(err.Error(), "gzip: invalid header") {
if len(sf) > 0 {
if rr, err = os.Open(sf); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to do this. And we do decide to keep it, we should close the file in defer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mangalaman93 mangalaman93 mangalaman93 requested changes

@meghalims meghalims Awaiting requested review from meghalims

@sanjayk-github-dev sanjayk-github-dev Awaiting requested review from sanjayk-github-dev

@harshil-goel harshil-goel Awaiting requested review from harshil-goel

@all-seeing-code all-seeing-code Awaiting requested review from all-seeing-code

@billprovince billprovince Awaiting requested review from billprovince

@joshua-goldstein joshua-goldstein Awaiting requested review from joshua-goldstein

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
area/testing Testing related issues go Pull requests that update Go code
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jbhamra1 @CLAassistant @mangalaman93 @dgraph-bot