presto icon indicating copy to clipboard operation
presto copied to clipboard

Re-enable Nessie namespace/table visibility tests

Open nastra opened this issue 2 years ago • 3 comments

testTableDataVisibility() and testNamespaceVisibility are both using the same namespace names (where namespace_one is created on the main branch), so it's possible that those tests interfere with each other if the tests run in parallel

nastra avatar Jul 29 '22 12:07 nastra

@beinan yes that makes perfect sense to do that. I added some cleanup after each test that deletes all branches/tags and also improved the overall isolation in the tests so that parallel runs don't interfere with each other

nastra avatar Aug 01 '22 09:08 nastra

@beinan turns out this isn't as trivial as I thought, because the query runner with nessie is usually created at the class level, while we clean up and therefore modify the underlying references at the test level (which means we're modifying the underlying state of the query runner). @ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

nastra avatar Aug 01 '22 14:08 nastra

@ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

Sure. I can check in my free time.

ajantha-bhat avatar Aug 02 '22 05:08 ajantha-bhat

ping @ajantha-bhat!

rohanpednekar avatar Oct 19 '22 23:10 rohanpednekar

@rohanpednekar : Thanks for the remainder. I will handle it this week.

ajantha-bhat avatar Oct 20 '22 04:10 ajantha-bhat

@beinan turns out this isn't as trivial as I thought, because the query runner with nessie is usually created at the class level, while we clean up and therefore modify the underlying references at the test level (which means we're modifying the underlying state of the query runner). @ajantha-bhat can you please do a follow-up on this PR and rewrite the test in a way so that everything is properly cleaned up and tests don't interfere with each other when they run in parallel?

It appears that @Test(singleThreaded = true) fixed the issue I was seeing, so this should be ready for a re-review @beinan

nastra avatar Oct 20 '22 10:10 nastra

@beinan, @rohanpednekar: We have added the clean-up in this PR itself today. So, there won't any follow PR from me for the same.

ajantha-bhat avatar Oct 20 '22 10:10 ajantha-bhat

@beinan could you re-review this one please?

nastra avatar Oct 27 '22 16:10 nastra

@beinan @ChunxuTang is this something that we can merge?

nastra avatar Nov 03 '22 17:11 nastra

@beinan any idea why a different and unrelated CI action fails randomly?

nastra avatar Nov 15 '22 15:11 nastra

@nastra Let me rerun the tests and see whether they could pass this time.

ChunxuTang avatar Nov 23 '22 19:11 ChunxuTang

@ChunxuTang looks like CI is passing, is this good to be merged then?

nastra avatar Nov 24 '22 07:11 nastra