Skip to content

Conversation

lauzadis
Copy link
Contributor

@lauzadis lauzadis commented Jan 30, 2025

  • Implement SystemDefaultProvider for Native
    • Note: System properties are always returned null / empty.
  • Add a commonized test suite for Platform

Issue #

Description of changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
… to Filesystem interface

0marperez and others added 30 commits December 16, 2024 11:30

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@lauzadis lauzadis marked this pull request as ready for review February 3, 2025 21:51
@lauzadis lauzadis requested a review from a team as a code owner February 3, 2025 21:51

This comment has been minimized.

Comment on lines +59 to +63
@Test
fun testOsInfo() = runTest {
val osInfo = PlatformProvider.System.osInfo()
assertNotEquals(OsFamily.Unknown, osInfo.family)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Do we have tests anywhere to verify that the correct OS family is returned as opposed to verifying the incorrect one isn't?

Copy link
Contributor Author

@lauzadis lauzadis Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't. Since this is a common test it's not possible to assert on a specific OS family. I'll try making a target-specific source set (i.e. linuxx64) and add a test there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a LinuxX64 test

Comment on lines 33 to 34
actual override suspend fun readFileOrNull(path: String): ByteArray? {
TODO("Not yet implemented")
return try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: The JVM impl dispatches to IO. Should this impl do the same? (similar question for writeFile)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion, I think it should

This comment has been minimized.

1 similar comment

This comment has been minimized.

}

@Test
fun definitelyShouldFail() = runTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this wasn't meant to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it here to make sure the Linux tests are running in CI. Will be removed


val family = when {
sysName.contains("darwin") -> {
if (machine.startsWith("iPhone") || machine.startsWith("iPad")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Can we cover Apple watches & TV's as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I added them along with a TODO to validate it actually gets resolved correctly

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Feb 4, 2025

Affected Artifacts

Changed in size
Artifact Pull Request (bytes) Latest Release (bytes) Delta (bytes) Delta (percentage)
http-test-jvm.jar 61,604 58,690 2,914 4.97%
aws-signing-default-jvm.jar 53,497 51,946 1,551 2.99%
crt-util-jvm.jar 21,451 20,952 499 2.38%
runtime-core-jvm.jar 836,052 818,814 17,238 2.11%
aws-signing-tests-jvm.jar 456,687 456,627 60 0.01%
test-suite-jvm.jar 96,930 97,206 -276 -0.28%

@lauzadis lauzadis merged commit ae80656 into kn-main Feb 4, 2025
20 checks passed
@lauzadis lauzadis deleted the kn-platform branch February 4, 2025 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a changelog entry isn't required for a pull request. Use sparingly.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants