-
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 systest/plugin #8896
Conversation
adb5749
to
4e90467
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.
Looks pretty good to go, just a few more comments. And you need to undo the comment you added around the definition of type testCase.
4e90467
to
770c3fd
Compare
b4fc0b2
to
dc06871
Compare
dc06871
to
2c42783
Compare
2c42783
to
f22c1e7
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.
A few minor comments, LGTM otherwise.
f22c1e7
to
02ac64f
Compare
cd9b84f
to
a25dd3f
Compare
a25dd3f
to
d86341f
Compare
ee932d6
to
58d1bc2
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.
Up to you if you want to reduce the 2 min wait to 30 second before killing the container.
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.
Would you mind providing some more details in the PR description about the motivation behind this change and a description of the tests? Additionally, I'd be grateful if you could help me understand the role of plugins within dgraph. An example here would be really insightful. Thank you!
58d1bc2
to
f8e82b5
Compare
…ContainerKill() called from it
f8e82b5
to
20936aa
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.
Few comments, rest looks good.
@@ -57,7 +58,9 @@ type LocalCluster struct { | |||
tempBinDir string | |||
tempSecretsDir string | |||
encKeyPath string | |||
lowerThanV21 bool | |||
|
|||
lowerThanV21 bool |
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 do we need this boolean?
This is an existing 'integration' test, now transformed to run for 'integration' and 'upgrade' using dgraphtest framework. Additionally, leveraging 'testify/suite' package's SetupTest() / SetupSubTest() / TeardownTest() / TeardownSubTest() functionality during the test runs. 'integration' tests are still run using t.go and the dgraphtest framework acts as a no-op for now. Going forward, there are plans to have dgraphtest framework take over the t.go. 'upgrade' used in the tests is Dgraph version upgrade from a source version to a target version using 'BackupRestore' and 'InPlace'.
Plugin insights: Hope this link will help here https://dgraph.io/docs/query-language/indexing-custom-tokenizers/#implementing-a-plugin
Closes: https://dgraph.atlassian.net/browse/DGRAPHCORE-304