Port more tests to REST CI #4838

ypatia · 2024-03-28T14:16:51Z

As per title, more tests in tiledb_unit are adapted to run on the REST CI runner.

After this PR is merged we'll have 160 tests ported to REST-CI runner, of which 143 will be running and passing and 17 will be disabled until the issues found and logged here:https://app.shortcut.com/tiledb-inc/story/40489/issues-found-while-running-tiledb-unit-core-tests-against-rest-cloud-server are fixed.

TYPE: NO_HISTORY
DESC: Port more tests to REST CI

shortcut-integration · 2024-03-28T14:16:54Z

This pull request has been linked to Shortcut Story #40376: Port another batch of tests to REST CI..

KiterLuc · 2024-03-29T11:00:36Z

test/src/unit-capi-nullable.cc

@@ -852,16 +842,6 @@ TEST_CASE_METHOD(
    NullableArrayFx,
    "C API: Test 2D array with nullable attributes",
    "[capi][2d][nullable]") {


Did we forget to enable this?

I will do it the next batch. There were failures I need to investigate and didn't want to block this already huge PR. Let me add a TODO not to forget.

KiterLuc · 2024-03-29T11:05:24Z

test/src/unit-capi-query-aggregate.cc

@@ -1,5 +1,5 @@
 /**
- * @file tiledb/api/c_api/query_aggregate/test/unit_capi_query_aggregate.cc


Let's revert this file... I have a work item to enable an incompletes test on REST CI for next sprint. Then we can delete the incompletes test from here. Also, we can remove the serialization tests from this file and REST enable the tests in test-cppapi-aggregates.cc, which will cover that gap.

KiterLuc · 2024-03-29T11:06:00Z

test/src/unit-capi-sparse_heter.cc

-  std::string array_name =
-      local_fs.file_prefix() + local_fs.temp_dir() + "sparse_array_heter";
-  create_temp_dir(local_fs.file_prefix() + local_fs.temp_dir());
+    "[capi][sparse][heter][float-int64][non-rest]") {


What is the non-rest tag?

It's that I have verified that this test cannot be ported to REST-CI because it's using functionality that's not supported, e.g. encyption, consolidation etc. My thought is that in this way we can see clearly which tests we didn't work on porting yet, and which can't be ported ([non-rest] tagged ones). Let me know if you don't like it.

KiterLuc · 2024-03-29T15:10:32Z

test/src/unit-capi-consolidation.cc

@@ -6355,13 +6213,6 @@ TEST_CASE_METHOD(
    ConsolidationFx,
    "C API: Test consolidating fragment metadata, sparse string",
    "[capi][consolidation][fragment-meta][sparse][string]") {


Did we forget the REST tag here?

yes, forgot to add this test since it applies to REST, good catch!

This turned out to need significant work as I needed to adapt the whole consolidation test suite so that I can run those few tests (fragment_metadata and commits consolidation) on REST CI. I did it because I think it's worth it as we can now very easily enable the rest of the tests to run on REST CI when we enable fragment consolidation on for remote arrays.

shaunrd0

A few comments / questions but nothing major, LGTM

shaunrd0 · 2024-04-05T19:21:53Z

test/src/unit-backwards_compat.cc

@@ -1247,17 +1247,6 @@ TEST_CASE(
    "Backwards compatibility: Upgrades an array of older version and "
    "write/read it",
    "[backwards-compat][upgrade-version][write-read-new-version]") {


So... Backwards compatibility... Since those tests require access to old arrays that are available locally but not on s3, I decided not to port them in rest-CI at this moment at least. We could consider doing that implementing some kind of copy to s3 but it didn't seem like a priority to me given the effort needed..

shaunrd0 · 2024-04-05T19:32:23Z

test/src/unit-capi-array.cc

  // TODO: refactor for each supported FS.
  std::string temp_dir = fs_vec_[0]->temp_dir();
+  std::string array_name = fs_vec_[0]->is_rest() ? "tiledb://unit/" : "";


Could we prepend this in SupportedFsS3::temp_dir if is_rest_ is true? Or maybe create a prefix() method? Feel free to disregard if it's too much trouble, it's just a thought for convenience. I just noticed we're repeating this check in lots of tests.

I see your pain, I also don't like this but at least it's noise just for old tests. In new tests using VfsTestSetup you get this through array_uri method. Prepending it SupportedFsS3::temp_dir wouldn't work unfortunately afaiu because of all the cleanup/create etc operations we do directly on S3. I will look into moving the array_uri method in SupportedFsS3 class to make it less ugly. Wait for my next patch please.

shaunrd0 · 2024-04-05T20:02:01Z

test/src/unit-capi-consolidation.cc

@@ -4598,13 +4520,6 @@ TEST_CASE_METHOD(
    ConsolidationFx,
    "C API: Test consolidation, dense",
    "[capi][consolidation][dense]") {


[rest]? There's a few other tests in this file that were updated but not tagged for (non-)rest. No worries if there's a reason I'm missing, just FYI.

yes, so fragment consolidation and encryption are not supported for remote arrays. So for these ones I just removed the serialization wrappers because it's useless to test and didn't include them in rest CI. I have thought of introducing the [non-rest] tag for those tests to indicate that it just can't be ported, but missed to use it everywhere, hence your reasonable question.

shaunrd0 · 2024-04-05T20:16:20Z

test/src/unit-capi-dense_array.cc

@@ -3382,12 +3189,13 @@ TEST_CASE_METHOD(
    DenseArrayFx,
    "C API: Test dense array, open array checks",
    "[capi][dense][open-array-checks]") {


shaunrd0 · 2024-04-05T20:51:44Z

test/src/unit-capi-string_dims.cc

-  std::string array_name =
-      local_fs.file_prefix() + local_fs.temp_dir() + "string_dims";
-  create_temp_dir(local_fs.file_prefix() + local_fs.temp_dir());
+    "[capi][sparse][string-dims][errors][rest-fails][new]") {


What is the [new] tag used for here? Just curious, it was added in a few other tests in this file as well.

That's a leftover from my testing, I was using it to only run a portion of the tests. Thanks for spotting it ! Removing and adding the right references to the issue opened

As per title, more tests in tiledb_unit are adapted to run on the REST CI runner. After this PR is merged we'll have 160 tests ported to REST-CI runner, of which 143 will be running and passing and 17 will be disabled until the issues found and logged here:https://app.shortcut.com/tiledb-inc/story/40489/issues-found-while-running-tiledb-unit-core-tests-against-rest-cloud-server are fixed. --- TYPE: NO_HISTORY DESC: Port more tests to REST CI (cherry picked from commit e0bc0dd)

As per title, more tests in tiledb_unit are adapted to run on the REST CI runner. After this PR is merged we'll have 160 tests ported to REST-CI runner, of which 143 will be running and passing and 17 will be disabled until the issues found and logged here:https://app.shortcut.com/tiledb-inc/story/40489/issues-found-while-running-tiledb-unit-core-tests-against-rest-cloud-server are fixed. --- TYPE: NO_HISTORY DESC: Port more tests to REST CI

ypatia requested review from shaunrd0 and KiterLuc March 28, 2024 15:23

ypatia added 10 commits March 29, 2024 11:35

Add query-condition tests to REST-CI

e75718f

Add aggregate and string dim to REST

2217f8c

Add capi array and attribute tests

00f6faa

Add CAPI dense array tests

b988b3a

Add more tests

19a901b

Add query tests

9ec496b

Add sparse tests

4f87313

Add string dim tests

dafd635

Add more tests

3972802

Attempt to fix tests

6399150

ypatia force-pushed the yt/sc-40376/rest_ci_add_tests_part2 branch 2 times, most recently from a8b2dc6 to bd38a42 Compare March 29, 2024 11:43

Various fixes to stabilize CI

326314a

ypatia force-pushed the yt/sc-40376/rest_ci_add_tests_part2 branch from bd38a42 to 326314a Compare March 29, 2024 12:13

ypatia added 3 commits March 29, 2024 13:09

Fix CI problem with submit_and_finalize

63c8497

Fix use after free in tests and others

2c18c45

Exclude test with parallel array open from REST CI

0d62ad4

KiterLuc reviewed Mar 29, 2024

View reviewed changes

ypatia force-pushed the yt/sc-40376/rest_ci_add_tests_part2 branch from 1a1d8f2 to 29c2f9e Compare April 2, 2024 08:43

Address @KiterLuc's comments

4736be1

ypatia force-pushed the yt/sc-40376/rest_ci_add_tests_part2 branch from 29c2f9e to 4736be1 Compare April 2, 2024 09:37

Fix new contexts shadowing vfs configuration

c240cb5

ypatia force-pushed the yt/sc-40376/rest_ci_add_tests_part2 branch from 8f5b080 to c240cb5 Compare April 5, 2024 11:35

ypatia added 2 commits April 5, 2024 12:09

Refactor to simplify and fix ASAN

91f3620

Fix another context without vfs config

835dcdc

shaunrd0 approved these changes Apr 5, 2024

View reviewed changes

Address @shaunrd0's comments

4eaec70

KiterLuc merged commit e0bc0dd into dev Apr 9, 2024
58 checks passed

KiterLuc deleted the yt/sc-40376/rest_ci_add_tests_part2 branch April 9, 2024 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port more tests to REST CI #4838

Port more tests to REST CI #4838

ypatia commented Mar 28, 2024 •

edited by KiterLuc

Loading

shortcut-integration bot commented Mar 28, 2024

KiterLuc Mar 29, 2024

ypatia Apr 1, 2024

KiterLuc Mar 29, 2024

ypatia Apr 2, 2024

KiterLuc Mar 29, 2024

ypatia Apr 1, 2024

KiterLuc Mar 29, 2024

ypatia Apr 1, 2024

ypatia Apr 2, 2024

shaunrd0 left a comment

shaunrd0 Apr 5, 2024

ypatia Apr 8, 2024

shaunrd0 Apr 5, 2024

ypatia Apr 8, 2024

shaunrd0 Apr 5, 2024

ypatia Apr 8, 2024

shaunrd0 Apr 5, 2024

shaunrd0 Apr 5, 2024

ypatia Apr 8, 2024 •

edited

Loading

		@@ -1,5 +1,5 @@
		/**
		* @file tiledb/api/c_api/query_aggregate/test/unit_capi_query_aggregate.cc

Port more tests to REST CI #4838

Port more tests to REST CI #4838

Conversation

ypatia commented Mar 28, 2024 • edited by KiterLuc Loading

shortcut-integration bot commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunrd0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypatia Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

ypatia commented Mar 28, 2024 •

edited by KiterLuc

Loading

ypatia Apr 8, 2024 •

edited

Loading