-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
test(upgrade): add upgrade tests for 21million dataset #8927
Conversation
098f872
to
352f44b
Compare
8acbe18
to
8c90797
Compare
519fc7e
to
822ccdd
Compare
edd5fee
to
c2e272b
Compare
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.
Also, you probably want to remove downloading 21million dataset in the t.go
systest/21million/live/run_test.go
Outdated
func cleanupAndExit(exitCode int) { | ||
_ = os.RemoveAll("./t") | ||
os.Exit(exitCode) | ||
func downloadDataFiles(testDir string) error { |
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 is also defined twice
4a718be
to
031aa56
Compare
c99cef0
to
2d12883
Compare
d0f19b8
to
2d7c5fb
Compare
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.
Just a few minor comments. Looks pretty good otherwise.
dc dgraphtest.Cluster | ||
lc *dgraphtest.LocalCluster | ||
uc dgraphtest.UpgradeCombo | ||
bulkDataDir string |
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.
nit: you could leave an empty line before this so that the spacing looks better.
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") |
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.
why are we not returning an error here?
@@ -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") |
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.
same
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()) |
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.
we should return an error here too
|
||
func QueriesFor21Million(dc dgraphtest.Cluster) error { |
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.
QueriesFor21Million -> RunTestQueriesFor21Million
and I think we should pass t object to this function so that we could do t.Run
inside.
dc dgraphtest.Cluster | ||
lc *dgraphtest.LocalCluster | ||
uc dgraphtest.UpgradeCombo | ||
liveDataDir string |
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.
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 { |
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 don't think we need to do this. And we do decide to keep it, we should close the file in defer.
…ementation to work with ASCII too.
2d7c5fb
to
b8261fa
Compare
This PR has been stale for 60 days and will be closed automatically in 7 days. Comment to keep it open. |
Closes: https://dgraph.atlassian.net/browse/DGRAPHCORE-302
Docs: NA