Nessie: Fix testcase failures #7320
Conversation
| @@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() { | |||
|
|
|||
| @Test | |||
| public void testListTables() { | |||
| catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema); | |||
| createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema); | |||
There was a problem hiding this comment.
why not just call catalog.createNamespace(Namespace.of("foo"), Collections.emptyMap()); at the beginning of these methods?
There was a problem hiding this comment.
Suggestion will work but I am following the style from other testcases in Nessie introduced in #7146
There was a problem hiding this comment.
The concern I have is that creating the namespace for each catalog instance might actually hide a bug. That's why I think it's better to create the namespace once before the test starts
There was a problem hiding this comment.
Fixed that creating namespace from another catalog part and still following the same style of other testcases. If still required to manually create a namespace, I can do that. But I don't think necessary.
| Assertions.assertThat(catalog.listTables(Namespace.of("foo"))) | ||
| .containsExactlyInAnyOrder(TableIdentifier.parse("foo.tbl1")); | ||
|
|
||
| // another client creates a table with the same nessie server | ||
| anotherCatalog.createTable(TableIdentifier.parse("foo.tbl2"), schema); | ||
| createTable(anotherCatalog, TableIdentifier.parse("foo.tbl2"), schema); |
There was a problem hiding this comment.
I don't think it's necessary to potentially create the namespace again here. Just calling catalog.createNamespace(Namespace.of("foo"), Collections.emptyMap()); at the beginning of the method should be enough.
Same applies for the other test methods in this class that fail.
There was a problem hiding this comment.
yeah, creating again from a different client is not required as namespace already exist. I removed it.
|
PR is ready for merging. |
|
Thanks @stevenzwu for merging :) |
|
merge this to fix the master branch build failure. thanks @ajantha-bhat for the fix and @nastra @Fokko @dimas-b for the review |
#6789 was merged recently and the PR didn't rebase before merging as there was no conflict.
But there was another PR that got merged #7146 which enforces the namespace creation. Hence the new tests failed.
Apologies for breaking the master build!