Js/device testing #6964

ironage · 2023-09-08T21:55:20Z

First steps towards #6923
In order to run all the tests on a device, they need to be combined into a single application. This means invoking the core, sync and object-store tests all from the same runner. This new target is called CombinedTests. I have done this in a way that makes object libraries from all the sources, so that a developer can switch between the CombinedTests and an individual runner without having to recompile everything. The restructuring surfaced a cross project compile dependency in the TestUtil library which I solved in the code by replacing the outdated sync::PrimaryKey concept with Mixed.

To prove that this works, I changed the iOS simulator check on Jenkins to run the combined test app. This showed where we have sync tests relying on unimplemented code for the sync server on device, so I added guards on those sync tests to only compile them if we are not on a mobile platform. This allows the rest of the sync tests to run on.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

ironage · 2023-09-12T17:02:17Z

test/test_sync.cpp

@@ -127,6 +127,7 @@ ClientHistory& get_history(DBRef db)
    return get_replication(db).get_history();
 }

+#if !REALM_MOBILE // the server is not implemented on devices


There are no functional changes to the code in this file, just moving tests under the !REALM_MOBILE flag if they used the server fixtures.

ironage · 2023-09-12T17:05:00Z

test/object-store/CMakeLists.txt

    c_api/c_api.cpp
-    c_api/c_api.c
+    c_api/c_api_file_tests.c


linking with cmake's OBJECT library exposed a bug that did not support two files of the same name because the system is looking for c_api.o and getting confused because it wasn't there (cmake generates two c_api_<id>.o files to differentiate the two)

tgoyne · 2023-09-13T00:09:14Z

Why do we need all of the tests combined into a single app? We can't just run each of the separate apps in parallel?

ironage · 2023-09-13T00:16:07Z

I'm not sure, that might be possible. @fealebenpae asked for this to help unblock him because it would simplify connecting to the device farm.

fealebenpae · 2023-09-15T13:16:36Z

test/CMakeLists.txt

+
+add_executable(CoreTests main.cpp test_all.cpp ${REQUIRED_TEST_FILES})
+target_sources(CoreTests PUBLIC $<TARGET_OBJECTS:CoreTestLib>)
+target_sources(CoreTests PUBLIC $<TARGET_OBJECTS:TestUtil>)


Can't we link against TestUtil as a normal library instead of just including the object files directly?

Yeah, my understanding is that object libraries are just a build time optimization for when the archive file is large enough that creating it takes a meaningful amount of time and space, but TestUtil hopefully is very small. If we're having to work around any of the object library quirks then we should probably just use a static library?

I reverted TestUtil back to a static library.

fealebenpae · 2023-09-15T13:19:24Z

test/CMakeLists.txt

+
+if(MSVC)
+    # increase the number of sections supported in an obj file for the heavily templated tests
+    target_compile_options(CombinedTests PRIVATE /bigobj)


Does this make sense on the CombinedTests target? It's not actually compiling the source files where /bigobj is needed, aren't those part of the *TestLib targets?

removed 👍

fealebenpae · 2023-09-15T13:21:01Z

test/CMakeLists.txt

+target_sources(CombinedTests PUBLIC $<TARGET_OBJECTS:ObjectStoreTestLib>)
+if(REALM_ENABLE_SYNC)
+    target_sources(CombinedTests PUBLIC $<TARGET_OBJECTS:SyncTestLib>)
+    target_link_libraries(CombinedTests ObjectStoreTestLib CoreTestLib SyncTestLib Sync SyncServer Storage TestUtil)


target_sources with the $<TARGET_OBJECTS:myTgt> generator expression explicitly includes the object files in the link phase, target_link_libraries where one of the targets is an OBJECT library implies it. Only one should be necessary, I think.

You're right. It turns out that target_sources just links the source files while target_link_libraries will do that as well as linking any transitively linked dependencies from the targets. The latter is what we want as it simplifies everything.

fealebenpae · 2023-09-15T13:25:12Z

test/util/CMakeLists.txt

@@ -50,7 +50,7 @@ if(REALM_ENABLE_SYNC)
    )
 endif()

-add_library(TestUtil STATIC ${TEST_UTIL_SOURCES} ${TEST_UTIL_HEADERS})
+add_library(TestUtil OBJECT ${TEST_UTIL_SOURCES} ${TEST_UTIL_HEADERS})


I don't think TestUtil has to be an OBJECT library, it can stay as static.

fealebenpae · 2023-09-15T13:37:41Z

In order to run this on an actual device on iOS we need to set up a proper app machinery with an entrypoint ViewController, I think, which only needs to call into combined_tests.cpp's main() in -viewDidLoad. AFAIK on iOS the system watchdog will kill the app if it hasn't started processing app lifecycle messages on its main thread within a few seconds so we can't just block in our main() without calling UIApplicationMain() with an appropriate delegate.

fealebenpae · 2023-09-15T13:40:03Z

@tgoyne we're gearing up for running tests on AWS Device Farm devices and it's gonna save us a lot of time per run if we run all the tests in a combined package, instead of in parallel, and it will save on boilerplate in CI to set the runs up as well. This additional overhead is why we never bothered to run sync tests on iOS or Android simulator/emulators until now, for example.

tgoyne · 2023-09-15T15:24:46Z

Why would it save any time per run?

The only reason we weren't running sync tests on iOS was because they needed the changes here to be able to compile on iOS, not because there was anything difficult about executing them. I had actually set everything up to run the sync tests in the ios simulator on Jenkins and then commented it out when I discovered that the tests didn't compile.

fealebenpae · 2023-09-15T15:34:00Z

Why would it save any time per run?

This is strictly for AWS Device Farm. When running the .NET tests on an Android device there the actual tests run in ~4 minutes, but the total step is ~9 minutes. I have seen similar overhead when doing manual tests with iOS devices on Device Farm. AWS just has some overhead for setting up and winding down tests on devices that we can avoid if we group tests together instead of in distinct executables.

tgoyne · 2023-09-15T15:38:52Z

Ah, that makes sense then.

coveralls-official · 2023-09-15T21:17:38Z

Pull Request Test Coverage Report for Build james.stone_439

433 of 500 (86.6%) changed or added relevant lines in 5 files are covered.
53 unchanged lines in 11 files lost coverage.
Overall coverage increased (+0.02%) to 91.681%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test/util/compare_groups.cpp	20	28	71.43%
test/object-store/test_runner.cpp	101	160	63.13%

Files with Coverage Reduction	New Missed Lines	%
src/realm/sync/noinst/client_impl_base.hpp	1	93.91%
src/realm/sync/noinst/server/server.cpp	1	76.02%
test/test_query2.cpp	1	99.08%
src/realm/table_view.cpp	2	94.18%
src/realm/sort_descriptor.cpp	3	93.7%
src/realm/util/file.cpp	3	81.73%
src/realm/alloc_slab.cpp	5	92.89%
src/realm/index_string.cpp	6	87.84%
src/realm/bplustree.cpp	7	75.72%
src/realm/sync/noinst/client_impl_base.cpp	10	85.13%

Totals
Change from base Build 1873:	0.02%
Covered Lines:	231791
Relevant Lines:	252824

💛 - Coveralls

ironage · 2023-09-19T21:32:07Z

In order to run this on an actual device on iOS we need to set up a proper app machinery with an entrypoint ViewController, I think, which only needs to call into combined_tests.cpp's main() in -viewDidLoad. AFAIK on iOS the system watchdog will kill the app if it hasn't started processing app lifecycle messages on its main thread within a few seconds so we can't just block in our main() without calling UIApplicationMain() with an appropriate delegate.

@fealebenpae I believe we can change test/unit-tests-ios/ViewController.mm to become like test/combined_tests.cpp and we should be good to go. Though I am not sure how to compile or run the project there. Do you know how, or should I reach out to the swift team for assistance?

tgoyne · 2023-09-21T16:32:46Z

I tried to run this locally on a device. It initially failed because the "Bundle name" field in Info.plist was empty. After manually setting that (and the development team) I was able to run the tests on device and it all passed. I'm not sure offhand how to set that field in cmake.

ironage · 2023-11-29T22:11:54Z

@tgoyne @fealebenpae how about we merge these build changes in as a first step. Then the final work to hook up devices can be done in a smaller PR when we get time for it? That would save this branch from going stale.

cla-bot bot added the cla: yes label Sep 8, 2023

ironage force-pushed the js/device-testing branch from 6ee55b9 to caef080 Compare September 12, 2023 16:55

add a combined test executable

d6d1726

ironage force-pushed the js/device-testing branch from caef080 to d6d1726 Compare September 12, 2023 16:59

ironage requested review from tgoyne and fealebenpae September 12, 2023 17:00

ironage self-assigned this Sep 12, 2023

ironage marked this pull request as ready for review September 12, 2023 17:00

ironage commented Sep 12, 2023

View reviewed changes

disable a recently added test on mobile

9c66d66

fealebenpae reviewed Sep 15, 2023

View reviewed changes

simplify link dependencies

486e253

ironage added 4 commits November 24, 2023 16:12

Merge branch 'master' into js/device-testing

b423c5c

add missing namespace

faf14a5

fix merge

ef24c23

fix object-store benchmarks on android

613a898

ironage force-pushed the js/device-testing branch from acebf36 to 613a898 Compare November 29, 2023 19:52

tgoyne approved these changes Nov 30, 2023

View reviewed changes

Merge branch 'master' into js/device-testing

fa8b285

ironage merged commit e593a5f into master Dec 1, 2023
2 checks passed

ironage deleted the js/device-testing branch December 1, 2023 01:56

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Js/device testing #6964

Js/device testing #6964

ironage commented Sep 8, 2023 •

edited

Loading

ironage Sep 12, 2023

ironage Sep 12, 2023 •

edited

Loading

tgoyne commented Sep 13, 2023

ironage commented Sep 13, 2023

fealebenpae Sep 15, 2023

tgoyne Sep 15, 2023

ironage Sep 15, 2023

fealebenpae Sep 15, 2023

ironage Sep 15, 2023

fealebenpae Sep 15, 2023

ironage Sep 15, 2023

fealebenpae Sep 15, 2023

fealebenpae commented Sep 15, 2023

fealebenpae commented Sep 15, 2023

tgoyne commented Sep 15, 2023

fealebenpae commented Sep 15, 2023

tgoyne commented Sep 15, 2023

coveralls-official bot commented Sep 15, 2023 •

edited

Loading

ironage commented Sep 19, 2023

tgoyne commented Sep 21, 2023

ironage commented Nov 29, 2023

Js/device testing #6964

Js/device testing #6964

Conversation

ironage commented Sep 8, 2023 • edited Loading

☑️ ToDos

Choose a reason for hiding this comment

ironage Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

tgoyne commented Sep 13, 2023

ironage commented Sep 13, 2023

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

fealebenpae commented Sep 15, 2023

fealebenpae commented Sep 15, 2023

tgoyne commented Sep 15, 2023

fealebenpae commented Sep 15, 2023

tgoyne commented Sep 15, 2023

coveralls-official bot commented Sep 15, 2023 • edited Loading

Pull Request Test Coverage Report for Build james.stone_439

💛 - Coveralls

ironage commented Sep 19, 2023

tgoyne commented Sep 21, 2023

ironage commented Nov 29, 2023

ironage commented Sep 8, 2023 •

edited

Loading

ironage Sep 12, 2023 •

edited

Loading

coveralls-official bot commented Sep 15, 2023 •

edited

Loading